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

Code before:
    UpdateVisuals();
}

// Method Description:
// - Sets the "Active" state on this Pane. Only one Pane in a tree of Panes
//   should be "active", and that pane should be a leaf.
// - Updates our visuals to match our new state, including highlighting our borders.
// Arguments:
// - <none>
// Return Value:
// - <none>
void Pane::SetActive()
{
    _lastActive = true;
    UpdateVisuals();
}

// Method Description:
// - Returns nullopt if no children of this pane were the last control to be
//   focused, or the GUID of the profile of the last control to be focused (if
//   there was one).
// Arguments:
// - <none>
// Return Value:
// - nullopt if no children of this pane were the last control to be
//   focused, else the GUID of the profile of the last control to be focused
Profile Pane::GetFocusedProfile()
{
    auto lastFocused = GetActivePane();
    return lastFocused ? lastFocused->_profile : nullptr;
}

// Method Description:
// - Returns true if this pane was the last pane to be focused in a tree of panes.
// Arguments:
// - <none>
// Return Value:
// - true iff we were the last pane focused in this tree of panes.
bool Pane::WasLastFocused() const noexcept
{
    return _lastActive;
}

// Method Description:
// - Returns true iff this pane has no child panes.
// Arguments:
// - <none>
// Return Value:
// - true iff this pane has no child panes.
bool Pane::_IsLeaf() const noexcept
{

Review comment (bug):
```suggestion
// - nullptr if no children of this pane were the last control to be
//   focused, else the profile of the last control to be focused
```

Code after:
    UpdateVisuals();
}

// Method Description:
// - Sets the "Active" state on this Pane. Only one Pane in a tree of Panes
//   should be "active", and that pane should be a leaf.
// - Updates our visuals to match our new state, including highlighting our borders.
// Arguments:
// - <none>
// Return Value:
// - <none>
void Pane::SetActive()
{
    _lastActive = true;
    UpdateVisuals();
}

// Method Description:
// - Returns nullptr if no children of this pane were the last control to be
//   focused, or the profile of the last control to be focused (if there was
//   one).
// Arguments:
// - <none>
// Return Value:
// - nullptr if no children of this pane were the last control to be
//   focused, else the profile of the last control to be focused
Profile Pane::GetFocusedProfile()
{
    auto lastFocused = GetActivePane();
    return lastFocused ? lastFocused->_profile : nullptr;
}

// Method Description:
// - Returns true if this pane was the last pane to be focused in a tree of panes.
// Arguments:
// - <none>
// Return Value:
// - true iff we were the last pane focused in this tree of panes.
bool Pane::WasLastFocused() const noexcept
{
    return _lastActive;
}

// Method Description:
// - Returns true iff this pane has no child panes.
// Arguments:
// - <none>
// Return Value:
// - true iff this pane has no child panes.
bool Pane::_IsLeaf() const noexcept
{
Repository: microsoft/terminal
File: src/cascadia/Remoting/WindowManager.cpp (C++)

Code before:
    // Arguments:
    // - <none>
    // Return Value:
    // - <none>
    winrt::fire_and_forget WindowManager::RequestShowTrayIcon()
    {
        co_await winrt::resume_background();
        _peasant.RequestShowTrayIcon();
    }

    // Method Description:
    // - Ask the monarch to hide its tray icon.
    // Arguments:
    // - <none>
    // Return Value:
    // - <none>
    winrt::fire_and_forget WindowManager::RequestHideTrayIcon()
    {
        auto strongThis{ get_strong() };
        co_await winrt::resume_background();
        _peasant.RequestHideTrayIcon();
    }

    bool WindowManager::DoesQuakeWindowExist()
    {
        const auto names = GetPeasants();
        for (const auto [id, p] : names)
        {
            if (p.WindowName() == QuakeWindowName)
            {
                return true;
            }
        }
        return false;
    }

    void WindowManager::UpdateTitle(winrt::hstring title)
    {
        _peasant.ActiveTabTitle(title);
    }
}


Review comment (suggestion):
Might be easier at this point to just have a `Monarch::DoesQuakeWindowExist`, so the `WindowManager` doesn't have to guard these queries to each peasant

Code after:
    // Arguments:
    // - <none>
    // Return Value:
    // - <none>
    winrt::fire_and_forget WindowManager::RequestShowTrayIcon()
    {
        co_await winrt::resume_background();
        _peasant.RequestShowTrayIcon();
    }

    // Method Description:
    // - Ask the monarch to hide its tray icon.
    // Arguments:
    // - <none>
    // Return Value:
    // - <none>
    winrt::fire_and_forget WindowManager::RequestHideTrayIcon()
    {
        auto strongThis{ get_strong() };
        co_await winrt::resume_background();
        _peasant.RequestHideTrayIcon();
    }

    bool WindowManager::DoesQuakeWindowExist()
    {
        return _monarch.DoesQuakeWindowExist();
    }

    void WindowManager::UpdateActiveTabTitle(winrt::hstring title)
    {
        winrt::get_self<implementation::Peasant>(_peasant)->ActiveTabTitle(title);
    }
}
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsModel/ActionArgs.cpp (C++)

Code before:
        // "Summon the Terminal window, name:\"{_Name}\""
        if (!Name().empty())
        {
            ss << L", name: ";
            ss << std::wstring_view(Name());
        }
        return winrt::hstring{ ss.str() };
    }

    winrt::hstring FocusPaneArgs::GenerateName() const
    {
        // "Focus pane {Id}"
        return winrt::hstring{
            fmt::format(std::wstring_view(RS_(L"FocusPaneCommandKey")),
                        Id())
        };
    }

    winrt::hstring MultipleActionsArgs::GenerateName() const
    {
        if (!_Name.empty())
        {
            return _Name;
        }

        return RS_(L"MultipleActionsMissingName");
    }
}


Review comment (suggestion):
This raises a good point - this is another example of an action that doesn't really make sense unless the user gives it a manual name. I think the precedent we have here is the `sendInput` action, we just leave the `name` as the empty string if it wasn't given one. What that should do is force the user to give it a name if they want to see it in the command palette, or they can leave it nameless and only use it via a keybinding. (Granted, this has also resulted in #10981).

I'd think we could just follow that precedent here too

Code after:
        }

        std::wstringstream ss;
        ss << std::wstring_view(RS_(L"GlobalSummonCommandKey"));

        // "Summon the Terminal window"
        // "Summon the Terminal window, name:\"{_Name}\""
        if (!Name().empty())
        {
            ss << L", name: ";
            ss << std::wstring_view(Name());
        }
        return winrt::hstring{ ss.str() };
    }

    winrt::hstring FocusPaneArgs::GenerateName() const
    {
        // "Focus pane {Id}"
        return winrt::hstring{
            fmt::format(std::wstring_view(RS_(L"FocusPaneCommandKey")),
                        Id())
        };
    }

    winrt::hstring MultipleActionsArgs::GenerateName() const
    {
        return L"";
    }
}
Repository: microsoft/terminal
File: tools/ReleaseEngineering/New-TerminalStackedChangelog.ps1 (PowerShell)

Code before:
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT license.

#################################
# New-TerminalStackedChangelog generates a markdown file with attribution
# over a set of revision ranges.
# Dustin uses it when he's writing changelogs.
# 
## For example, generating the changelogs for both 1.9 and 1.10 might look like this:
# $ New-TerminalStackedChangelog 1.9..release-1.9 1.10..release-1.10
## The output will be a markdown-like document. Each commit will be converted into a
## single-line entry with attribution and an optional count:
#
# * Frob the foo (thanks @PanosP!)
# * [2] Fix the bug
#
# Entries with a count were present in both changelists.

[CmdletBinding()]
Param(
    [string[]]$RevisionRanges
)

Function Test-MicrosoftPerson($email) {
    Return $email -like "*@microsoft.com" -Or $email -like "pankaj.d*"
}

Function Generate-Thanks($Entry) {
	If ($_.Microsoft) {
		""
	} ElseIf (-Not [string]::IsNullOrEmpty($_.PossibleUsername)) {
		" (thanks @{0}!)" -f $_.PossibleUsername
	} Else {
		" (thanks @<{0}>!)" -f $_.Email
	}

Review comment (suggestion):
```suggestion
# $ New-TerminalStackedChangelog 1.9..release-1.9, 1.10..release-1.10
```
thanks based powershell

Code after:
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT license.

#################################
# New-TerminalStackedChangelog generates a markdown file with attribution
# over a set of revision ranges.
# Dustin uses it when he's writing the changelog.
# 
## For example, generating the changelog for both 1.9 and 1.10 might look like this:
# $ New-TerminalStackedChangelog 1.9..release-1.9, 1.10..release-1.10
## The output will be a markdown-like document. Each commit will be converted into a
## single-line entry with attribution and an optional count:
#
# * Foo the bar (thanks @PanosP!)
# * [2] Fix the bug
#
# Entries with a count were present in both changelists.
#
# If you don't have the release tags/branches checked out locally, you might
# need to do something like:
#
# $ New-TerminalStackedChangelog origin/release-1.8..origin/release-1.9

[CmdletBinding()]
Param(
    [string[]]$RevisionRanges
)

Function Test-MicrosoftPerson($email) {
    Return $email -like "*@microsoft.com" -Or $email -like "pankaj.d*"
}

Function Generate-Thanks($Entry) {
    # We don't need to thank ourselves for doing our jobs
    If ($_.Microsoft) {
Repository: microsoft/terminal
File: src/cascadia/TerminalApp/CommandPalette.cpp (C++)

Code before:
        {
            if (parsedCommands.size() >= CommandLineHistoryLength)
            {
                // Don't load more than CommandLineHistoryLength commands
                break;
            }

            if (const auto parsedCommand = _buildCommandLineCommand(c))
            {
                parsedCommands.push_back(*parsedCommand);
            }
        }
        return single_threaded_vector(std::move(parsedCommands));
    }

    // Method Description:
    // - Update recent commands by putting the provided command as most recent.
    // Upon race condition might override an update made by another window.
    // Return Value:
    // - <none>
    void CommandPalette::_updateRecentCommands(const hstring& command)
    {
        const auto recentCommands = ApplicationState::SharedInstance().RecentCommands();
        // If there aren't and recent commands already in the state, then we
        // don't need to copy any.
        const auto countToCopy = std::min(recentCommands? recentCommands.Size() : 0, CommandLineHistoryLength - 1);
        std::vector<hstring> newRecentCommands{ countToCopy + 1 };
        til::at(newRecentCommands, 0) = command;
        if (countToCopy)
        {
            recentCommands.GetMany(0, { newRecentCommands.data() + 1, countToCopy });
        }
        ApplicationState::SharedInstance().RecentCommands(single_threaded_vector(std::move(newRecentCommands)));
    }
}


Review comment (suggestion):
```suggestion
        const auto countToCopy = std::min(recentCommands ? recentCommands.Size() : 0, CommandLineHistoryLength - 1);
```

Code after:
        {
            if (parsedCommands.size() >= CommandLineHistoryLength)
            {
                // Don't load more than CommandLineHistoryLength commands
                break;
            }

            if (const auto parsedCommand = _buildCommandLineCommand(c))
            {
                parsedCommands.push_back(*parsedCommand);
            }
        }
        return single_threaded_vector(std::move(parsedCommands));
    }

    // Method Description:
    // - Update recent commands by putting the provided command as most recent.
    // Upon race condition might override an update made by another window.
    // Return Value:
    // - <none>
    void CommandPalette::_updateRecentCommands(const hstring& command)
    {
        const auto recentCommands = ApplicationState::SharedInstance().RecentCommands();
        // If there aren't and recent commands already in the state, then we
        // don't need to copy any.
        const auto countToCopy = std::min(recentCommands ? recentCommands.Size() : 0, CommandLineHistoryLength - 1);
        std::vector<hstring> newRecentCommands{ countToCopy + 1 };
        til::at(newRecentCommands, 0) = command;
        if (countToCopy)
        {
            recentCommands.GetMany(0, { newRecentCommands.data() + 1, countToCopy });
        }
        ApplicationState::SharedInstance().RecentCommands(single_threaded_vector(std::move(newRecentCommands)));
    }
}
Repository: microsoft/terminal
File: src/cascadia/TerminalCore/ColorFix.hpp (C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

// We probably want to include something else here, whatever gives us COLORREF and BYTE
#include "../../terminal/input/terminalInput.hpp"

struct ColorFix
{
public:
    ColorFix(COLORREF color);

    static double GetDeltaE(ColorFix x1, ColorFix x2);
    static COLORREF GetPerceivableColor(COLORREF fg, COLORREF bg);

    // RGB
    union
    {
        struct
        {
            BYTE r, g, b, dummy;
        };
        COLORREF rgb;
    };

    // Lab
    struct
    {
        double L, A, B;
    };


Review comment (suggestion):
This sounds like a TODO. Not sure what includes `COLORREF` tbh. Maybe take a look at `til::color`? I think we need `COLORREF` there.

Code after:
/*++
Copyright (c) Microsoft Corporation
Licensed under the MIT license.

Module Name:
- ColorFix

Abstract:
- Implementation of perceptual color nudging, which allows the Terminal
  to slightly shift the foreground color to make it more perceivable on
  the current background (for cases where the foreground is very close
  to being imperceivable on the background).

Author(s):
- Pankaj Bhojwani - Sep 2021

--*/

#pragma once

struct ColorFix
{
public:
    ColorFix(COLORREF color);

    static COLORREF GetPerceivableColor(COLORREF fg, COLORREF bg);

    // RGB
    union
    {
        struct
        {
Repository: microsoft/terminal
File: src/cascadia/TerminalCore/ColorFix.hpp (C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

// We probably want to include something else here, whatever gives us COLORREF and BYTE
#include "../../terminal/input/terminalInput.hpp"

struct ColorFix
{
public:
    ColorFix(COLORREF color);

    static double GetDeltaE(ColorFix x1, ColorFix x2);
    static COLORREF GetPerceivableColor(COLORREF fg, COLORREF bg);

    // RGB
    union
    {
        struct
        {
            BYTE r, g, b, dummy;
        };
        COLORREF rgb;
    };

    // Lab

Review comment (refactor):
I would add an abstract here just saying that it relates to the new setting and what we use this code for. Pretty minor.

Code after:
/*++
Copyright (c) Microsoft Corporation
Licensed under the MIT license.

Module Name:
- ColorFix

Abstract:
- Implementation of perceptual color nudging, which allows the Terminal
  to slightly shift the foreground color to make it more perceivable on
  the current background (for cases where the foreground is very close
  to being imperceivable on the background).

Author(s):
- Pankaj Bhojwani - Sep 2021

--*/

#pragma once

struct ColorFix
{
public:
    ColorFix(COLORREF color);

    static COLORREF GetPerceivableColor(COLORREF fg, COLORREF bg);
Repository: microsoft/terminal
File: src/cascadia/TerminalCore/ColorFix.cpp (C++)

Code before:
#include "pch.h"

#include <Windows.h>
#include <math.h>
#include "ColorFix.hpp"

const double gMinThreshold = 12.0;
const double gExpThreshold = 20.0;
const double gLStep = 5.0;

constexpr double rad006 = 0.104719755119659774;
constexpr double rad025 = 0.436332312998582394;
constexpr double rad030 = 0.523598775598298873;
constexpr double rad060 = 1.047197551196597746;
constexpr double rad063 = 1.099557428756427633;
constexpr double rad180 = 3.141592653589793238;
constexpr double rad275 = 4.799655442984406336;
constexpr double rad360 = 6.283185307179586476;

ColorFix::ColorFix(COLORREF color)
{
    rgb = color;
    _ToLab();
}

// Method Description:
// - Helper function to calculate HPrime
double ColorFix::_GetHPrimeFn(double x, double y)
{
    if (x == 0 && y == 0)
    {
        return 0;
    }

    const auto hueAngle = atan2(x, y);
    return hueAngle >= 0 ? hueAngle : hueAngle + rad360;
}

// Method Description:
// - Given 2 colors, computes the DeltaE value between them
// Arguments:
// - x1: the first color
// - x2: the second color

Review comment (suggestion):
maybe these (and the above) should be `static` too? Idk if that does anything though haha

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

// A lot of code was taken from
// https://github.com/Maximus5/ConEmu/blob/master/src/ConEmu/ColorFix.cpp
// and then adjusted to fit our style guidelines

#include "pch.h"

#include <Windows.h>
#include "ColorFix.hpp"

static constexpr double gMinThreshold = 12.0;
static constexpr double gExpThreshold = 20.0;
static constexpr double gLStep = 5.0;

static constexpr double rad006 = 0.104719755119659774;
static constexpr double rad025 = 0.436332312998582394;
static constexpr double rad030 = 0.523598775598298873;
static constexpr double rad060 = 1.047197551196597746;
static constexpr double rad063 = 1.099557428756427633;
static constexpr double rad180 = 3.141592653589793238;
static constexpr double rad275 = 4.799655442984406336;
static constexpr double rad360 = 6.283185307179586476;

ColorFix::ColorFix(COLORREF color)
{
    rgb = color;
    _ToLab();
}

// Method Description:
// - Helper function to calculate HPrime
double ColorFix::_GetHPrimeFn(double x, double y)
{
    if (x == 0 && y == 0)
    {
        return 0;
    }

    const auto hueAngle = atan2(x, y);
    return hueAngle >= 0 ? hueAngle : hueAngle + rad360;
}
Repository: microsoft/terminal
File: src/cascadia/TerminalCore/ColorFix.hpp (C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

// We probably want to include something else here, whatever gives us COLORREF and BYTE
#include "../../terminal/input/terminalInput.hpp"

struct ColorFix
{
public:
    ColorFix(COLORREF color);

    static double GetDeltaE(ColorFix x1, ColorFix x2);
    static COLORREF GetPerceivableColor(COLORREF fg, COLORREF bg);

    // RGB
    union
    {
        struct
        {
            BYTE r, g, b, dummy;
        };
        COLORREF rgb;
    };

    // Lab
    struct
    {
        double L, A, B;
    };

private:
    static double _GetHPrimeFn(double x, double y);
    void _ToLab();
    void _ToRGB();
    BYTE _Clamp(double v);
};


Review comment (suggestion):
Looks like `GetDeltaE` is only used in `ColorFix.cpp`. You may be able to make this `private`

Code after:
/*++
Copyright (c) Microsoft Corporation
Licensed under the MIT license.

Module Name:
- ColorFix

Abstract:
- Implementation of perceptual color nudging, which allows the Terminal
  to slightly shift the foreground color to make it more perceivable on
  the current background (for cases where the foreground is very close
  to being imperceivable on the background).

Author(s):
- Pankaj Bhojwani - Sep 2021

--*/

#pragma once

struct ColorFix
{
public:
    ColorFix(COLORREF color);

    static COLORREF GetPerceivableColor(COLORREF fg, COLORREF bg);

    // RGB
    union
    {
        struct
        {
            BYTE r, g, b, dummy;
        };
        COLORREF rgb;
    };

    // Lab
    struct
Repository: microsoft/terminal
File: src/cascadia/TerminalCore/ColorFix.cpp (C++)

Code before:
#include "pch.h"

#include <Windows.h>
#include <math.h>
#include "ColorFix.hpp"

const double gMinThreshold = 12.0;
const double gExpThreshold = 20.0;
const double gLStep = 5.0;

constexpr double rad006 = 0.104719755119659774;
constexpr double rad025 = 0.436332312998582394;
constexpr double rad030 = 0.523598775598298873;
constexpr double rad060 = 1.047197551196597746;
constexpr double rad063 = 1.099557428756427633;
constexpr double rad180 = 3.141592653589793238;
constexpr double rad275 = 4.799655442984406336;
constexpr double rad360 = 6.283185307179586476;

ColorFix::ColorFix(COLORREF color)
{
    rgb = color;
    _ToLab();
}

// Method Description:
// - Helper function to calculate HPrime
double ColorFix::_GetHPrimeFn(double x, double y)
{

Review comment (suggestion):
Looks like we already have `#include <cmath.h>` in `LibraryIncludes.h` (already included via the `pch.h` above). Curious what you need `math` for.

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

// A lot of code was taken from
// https://github.com/Maximus5/ConEmu/blob/master/src/ConEmu/ColorFix.cpp
// and then adjusted to fit our style guidelines

#include "pch.h"

#include <Windows.h>
#include "ColorFix.hpp"

static constexpr double gMinThreshold = 12.0;
static constexpr double gExpThreshold = 20.0;
static constexpr double gLStep = 5.0;

static constexpr double rad006 = 0.104719755119659774;
static constexpr double rad025 = 0.436332312998582394;
static constexpr double rad030 = 0.523598775598298873;
static constexpr double rad060 = 1.047197551196597746;
static constexpr double rad063 = 1.099557428756427633;
static constexpr double rad180 = 3.141592653589793238;
static constexpr double rad275 = 4.799655442984406336;
static constexpr double rad360 = 6.283185307179586476;

ColorFix::ColorFix(COLORREF color)
{
    rgb = color;
    _ToLab();