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

Code before:
    {
        _titlebar.Visibility(_IsTitlebarVisible() ? Visibility::Visible : Visibility::Collapsed);
    }
    // GH#4224 - When the auto-hide taskbar setting is enabled, then we don't
    // always get another window message to trigger us to remove the drag bar.
    // So, make sure to update the size of the drag region here, so that it
    // _definitely_ goes away.
    _ResizeDragBarWindow();
}

// Method Description:
// - Returns true if the titlebar is visible. For things like fullscreen mode,
//   borderless mode (aka "focus mode"), this will return false.
// Arguments:
// - <none>
// Return Value:
// - true iff the titlebar is visible
bool NonClientIslandWindow::_IsTitlebarVisible() const
{
    return !(_fullscreen || _borderless);
}

void NonClientIslandWindow::SetTitlebarBackground(winrt::Windows::UI::Xaml::Media::Brush brush)
{
    _titlebar.Background(brush);
    _titlebar.SetBackgroundBrush(brush);
}


Review comment (suggestion):
Should `.Background()` just be inside `SetBackgroundBrush`? Also, there's now _two setters for the background brush :/_

Code after:
    {
        _titlebar.Visibility(_IsTitlebarVisible() ? Visibility::Visible : Visibility::Collapsed);
    }
    // GH#4224 - When the auto-hide taskbar setting is enabled, then we don't
    // always get another window message to trigger us to remove the drag bar.
    // So, make sure to update the size of the drag region here, so that it
    // _definitely_ goes away.
    _ResizeDragBarWindow();
}

// Method Description:
// - Returns true if the titlebar is visible. For things like fullscreen mode,
//   borderless mode (aka "focus mode"), this will return false.
// Arguments:
// - <none>
// Return Value:
// - true iff the titlebar is visible
bool NonClientIslandWindow::_IsTitlebarVisible() const
{
    return !(_fullscreen || _borderless);
}

void NonClientIslandWindow::SetTitlebarBackground(winrt::Windows::UI::Xaml::Media::Brush brush)
{
    _titlebar.Background(brush);
}
Repository: microsoft/terminal
File: src/cascadia/WindowsTerminal/IslandWindow.cpp (C++)

Code before:
    {
        _OnGetMinMaxInfo(wparam, lparam);
        return 0;
    }
    case WM_CREATE:
    {
        _HandleCreateWindow(wparam, lparam);
        return 0;
    }
    case WM_SETFOCUS:
    {
        if (_interopWindowHandle != nullptr)
        {
            // send focus to the child window
            SetFocus(_interopWindowHandle);
            return 0;
        }
        break;
    }
    case WM_ACTIVATE:
    {
        // wparam = 0 indicates the window was deactivated
        const bool activated = LOWORD(wparam) != 0;
        _WindowActivatedHandlers(activated);

        if (_isQuakeWindow && _autoHideQuakeWindow && LOWORD(wparam) == 0)
        {
            HideWindow();
        }

        break;
    }

    case WM_NCLBUTTONDOWN:
    case WM_NCLBUTTONUP:
    case WM_NCMBUTTONDOWN:
    case WM_NCMBUTTONUP:
    case WM_NCRBUTTONDOWN:
    case WM_NCRBUTTONUP:
    case WM_NCXBUTTONDOWN:
    case WM_NCXBUTTONUP:
    {
        // If we clicked in the titlebar, raise an event so the app host can
        // dispatch an appropriate event.
        _DragRegionClickedHandlers();
        break;
    }
    case WM_MENUCHAR:
    {
        // GH#891: return this LRESULT here to prevent the app from making a
        // bell when alt+key is pressed. A menu is active and the user presses a

Review comment (suggestion):
This is a super nit

```suggestion
        if (_isQuakeWindow && _autoHideQuakeWindow && !activated)
```

Code after:
    {
        _OnGetMinMaxInfo(wparam, lparam);
        return 0;
    }
    case WM_CREATE:
    {
        _HandleCreateWindow(wparam, lparam);
        return 0;
    }
    case WM_SETFOCUS:
    {
        if (_interopWindowHandle != nullptr)
        {
            // send focus to the child window
            SetFocus(_interopWindowHandle);
            return 0;
        }
        break;
    }
    case WM_ACTIVATE:
    {
        // wparam = 0 indicates the window was deactivated
        const bool activated = LOWORD(wparam) != 0;
        _WindowActivatedHandlers(activated);

        if (_autoHideWindow && !activated)
        {
            if (_isQuakeWindow || _minimizeToNotificationArea)
            {
                HideWindow();
            }
            else
            {
                ShowWindow(GetHandle(), SW_MINIMIZE);
            }
        }

        break;
    }

    case WM_NCLBUTTONDOWN:
    case WM_NCLBUTTONUP:
    case WM_NCMBUTTONDOWN:
    case WM_NCMBUTTONUP:
    case WM_NCRBUTTONDOWN:
    case WM_NCRBUTTONUP:
    case WM_NCXBUTTONDOWN:
    case WM_NCXBUTTONUP:
    {
        // If we clicked in the titlebar, raise an event so the app host can
        // dispatch an appropriate event.
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp (C++)

Code before:
        });
        co_await winrt::resume_background();
        result = DefaultTerminal::Available();
    }();

    latch.wait();
    _defaultTerminals = winrt::single_threaded_observable_vector(std::move(result.first));
    _currentDefaultTerminal = std::move(result.second);
}

void CascadiaSettings::ExportFile(winrt::hstring path, winrt::hstring content)
{
    try
    {
        WriteUTF8FileAtomic({ path.c_str() }, til::u16u8(content));
    }
    CATCH_LOG();
}

void CascadiaSettings::_validateThemeExists()
{
    if (_globals->Themes().Size() == 0)
    {
        // GH #13474
        // We didn't even load the default themes. This should only be possible
        // in the tests that don't even pass in defaults.json. No matter. Create
        // a default theme under `system` and just stick it in there.
        auto newTheme = winrt::make_self<Theme>();
        newTheme->Name(L"system");
        _globals->AddTheme(*newTheme);
        _globals->Theme(L"system");
    }

    if (!_globals->Themes().HasKey(_globals->Theme()))
    {
        _warnings.Append(SettingsLoadWarnings::UnknownTheme);

        // safely fall back to system as the theme.
        _globals->Theme(L"system");
    }
}


Review comment (suggestion):
Comments that say "this never happens" _do not age well_; reconsider!

(also: linking it to a github ID isn't meaningful in this case. we just should always make sure there's a theme, regardless of a github issue. I generally hold that if we take this to its logical extreme, every line of code will have its own github issue . . . and then it's no better than `git blame` but in many ways it is worse)

Code after:
        });
        co_await winrt::resume_background();
        result = DefaultTerminal::Available();
    }();

    latch.wait();
    _defaultTerminals = winrt::single_threaded_observable_vector(std::move(result.first));
    _currentDefaultTerminal = std::move(result.second);
}

void CascadiaSettings::ExportFile(winrt::hstring path, winrt::hstring content)
{
    try
    {
        WriteUTF8FileAtomic({ path.c_str() }, til::u16u8(content));
    }
    CATCH_LOG();
}

void CascadiaSettings::_validateThemeExists()
{
    if (_globals->Themes().Size() == 0)
    {
        // We didn't even load the default themes. This should only be possible
        // if the defaults.json didn't include any themes, or if no
        // defaults.json was loaded at all. The second case is especially common
        // in tests (that don't bother with a defaults.json). No matter. Create
        // a default theme under `system` and just stick it in there.

        auto newTheme = winrt::make_self<Theme>();
        newTheme->Name(L"system");
        _globals->AddTheme(*newTheme);
        _globals->Theme(L"system");
    }

    if (!_globals->Themes().HasKey(_globals->Theme()))
    {
        _warnings.Append(SettingsLoadWarnings::UnknownTheme);

        // safely fall back to system as the theme.
        _globals->Theme(L"system");
    }
}
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsEditor/GlobalAppearance.cpp (C++)

Code before:
using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Xaml::Navigation;
using namespace winrt::Windows::UI::Xaml::Controls;
using namespace winrt::Microsoft::Terminal::Settings::Model;
using namespace winrt::Windows::Foundation::Collections;

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    GlobalAppearance::GlobalAppearance()
    {
        InitializeComponent();
    }

    void GlobalAppearance::OnNavigatedTo(const NavigationEventArgs& e)
    {
        _ViewModel = e.Parameter().as<Editor::GlobalAppearanceViewModel>();
        AlwaysShowTabsToggleSwitch().IsEnabled(!_ViewModel.ShowTabsInTitlebar());
    }

    void GlobalAppearance::ShowTitleBarToggled(const IInspectable& /* sender */, const RoutedEventArgs& /* args */)
    {
        AlwaysShowTabsToggleSwitch().IsEnabled(!ShowTitlebarToggleSwitch().IsOn());

        if (ShowTitlebarToggleSwitch().IsOn())
        {
            AlwaysShowTabsToggleSwitch().IsOn(true);
        }
    }
}


Review comment (performance):
In general, you'll want to edit the setting directly (using `AlwaysShowTabs(true)`) instead of making the switch do it because it reduces code complexity. Right now, this adds a dependency on AlwaysShowTabs being a named switch... but if you use the setting on the ViewModel directly, you can avoid naming the switch at all because the setting is _bound to_ the switch. :smile:

The same thing is true of the `ShowTitlebarToggleSwitch`; if you use the `ShowTitlebar` setting (or whatever it's called,) you can get closer to the data model and avoid permanently naming the switch!

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "pch.h"
#include "GlobalAppearance.h"
#include "GlobalAppearance.g.cpp"

#include <LibraryResources.h>
#include <WtExeUtils.h>

using namespace winrt;
using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Xaml::Navigation;
using namespace winrt::Windows::UI::Xaml::Controls;
using namespace winrt::Microsoft::Terminal::Settings::Model;
using namespace winrt::Windows::Foundation::Collections;

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    GlobalAppearance::GlobalAppearance()
    {
        InitializeComponent();
    }

    void GlobalAppearance::OnNavigatedTo(const NavigationEventArgs& e)
    {
        _ViewModel = e.Parameter().as<Editor::GlobalAppearanceViewModel>();
    }
}
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsEditor/GlobalAppearance.cpp (C++)

Code before:

#include "pch.h"
#include "GlobalAppearance.h"
#include "GlobalAppearance.g.cpp"

#include <LibraryResources.h>
#include <WtExeUtils.h>

using namespace winrt;
using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Xaml::Navigation;
using namespace winrt::Windows::UI::Xaml::Controls;
using namespace winrt::Microsoft::Terminal::Settings::Model;
using namespace winrt::Windows::Foundation::Collections;

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    GlobalAppearance::GlobalAppearance()
    {
        InitializeComponent();
    }

    void GlobalAppearance::OnNavigatedTo(const NavigationEventArgs& e)
    {
        _ViewModel = e.Parameter().as<Editor::GlobalAppearanceViewModel>();
        AlwaysShowTabsToggleSwitch().IsEnabled(!_ViewModel.ShowTabsInTitlebar());
    }
}


Review comment (suggestion):
Now that you have `IsEnabled` in the xaml file, you don't need this line any more ... unless the XAML is not working properly! :)

Code after:

#include "pch.h"
#include "GlobalAppearance.h"
#include "GlobalAppearance.g.cpp"

#include <LibraryResources.h>
#include <WtExeUtils.h>

using namespace winrt;
using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Xaml::Navigation;
using namespace winrt::Windows::UI::Xaml::Controls;
using namespace winrt::Microsoft::Terminal::Settings::Model;
using namespace winrt::Windows::Foundation::Collections;

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    GlobalAppearance::GlobalAppearance()
    {
        InitializeComponent();
    }

    void GlobalAppearance::OnNavigatedTo(const NavigationEventArgs& e)
    {
        _ViewModel = e.Parameter().as<Editor::GlobalAppearanceViewModel>();
    }
}
Repository: microsoft/terminal
File: src/cascadia/TerminalControl/TermControlAutomationPeer.cpp (C++)

Code before:
        {
        case PatternInterface::Text:
            return *this;
            break;
        default:
            return nullptr;
        }
    }

    AutomationOrientation TermControlAutomationPeer::GetOrientationCore() const
    {
        return AutomationOrientation::Vertical;
    }

    hstring TermControlAutomationPeer::GetNameCore() const
    {
        // fallback to title if profile name is empty
        if (auto control{ _termControl.get() })
        {
            auto profileName = control->GetProfileName();
            if (profileName.empty())
            {
                return control->Title();
            }
            else
                return profileName;
        }

        return L"";
    }

    hstring TermControlAutomationPeer::GetHelpTextCore() const
    {
        if (auto control{ _termControl.get() })
        {
            return control->Title();
        }
        return L"";
    }

    AutomationLiveSetting TermControlAutomationPeer::GetLiveSettingCore() const
    {
        return AutomationLiveSetting::Polite;
    }

#pragma region ITextProvider
    com_array<XamlAutomation::ITextRangeProvider> TermControlAutomationPeer::GetSelection()
    {
        return _contentAutomationPeer.GetSelection();
    }


Review comment (suggestion):
```suggestion
            else
            {
                return profileName;
            }
```

Code after:
        {
        case PatternInterface::Text:
            return *this;
            break;
        default:
            return nullptr;
        }
    }

    AutomationOrientation TermControlAutomationPeer::GetOrientationCore() const
    {
        return AutomationOrientation::Vertical;
    }

    hstring TermControlAutomationPeer::GetNameCore() const
    {
        // fallback to title if profile name is empty
        if (auto control{ _termControl.get() })
        {
            const auto profileName = control->GetProfileName();
            if (profileName.empty())
            {
                return control->Title();
            }
            else
            {
                return profileName;
            }
        }

        return L"";
    }

    hstring TermControlAutomationPeer::GetHelpTextCore() const
    {
        if (const auto control{ _termControl.get() })
        {
            return control->Title();
        }
        return L"";
    }

    AutomationLiveSetting TermControlAutomationPeer::GetLiveSettingCore() const
    {
        return AutomationLiveSetting::Polite;
    }

#pragma region ITextProvider
    com_array<XamlAutomation::ITextRangeProvider> TermControlAutomationPeer::GetSelection()
    {
        return _contentAutomationPeer.GetSelection();
Repository: microsoft/terminal
File: src/cascadia/TerminalControl/TermControlAutomationPeer.cpp (C++)

Code before:
        return RS_(L"TerminalControl_ControlType");
    }

    Windows::Foundation::IInspectable TermControlAutomationPeer::GetPatternCore(PatternInterface patternInterface) const
    {
        switch (patternInterface)
        {
        case PatternInterface::Text:
            return *this;
            break;
        default:
            return nullptr;
        }
    }

    AutomationOrientation TermControlAutomationPeer::GetOrientationCore() const
    {
        return AutomationOrientation::Vertical;
    }

    hstring TermControlAutomationPeer::GetNameCore() const
    {
        // fallback to title if profile name is empty
        if (auto control{ _termControl.get() })
        {
            auto profileName = control->GetProfileName();
            if (profileName.empty())
            {
                return control->Title();
            }
            else
                return profileName;
        }

        return L"";
    }

    hstring TermControlAutomationPeer::GetHelpTextCore() const
    {
        if (auto control{ _termControl.get() })
        {
            return control->Title();
        }
        return L"";
    }

    AutomationLiveSetting TermControlAutomationPeer::GetLiveSettingCore() const
    {
        return AutomationLiveSetting::Polite;
    }


Review comment (suggestion):
```suggestion
            const auto profileName = control->GetProfileName();
```

Code after:
        return RS_(L"TerminalControl_ControlType");
    }

    Windows::Foundation::IInspectable TermControlAutomationPeer::GetPatternCore(PatternInterface patternInterface) const
    {
        switch (patternInterface)
        {
        case PatternInterface::Text:
            return *this;
            break;
        default:
            return nullptr;
        }
    }

    AutomationOrientation TermControlAutomationPeer::GetOrientationCore() const
    {
        return AutomationOrientation::Vertical;
    }

    hstring TermControlAutomationPeer::GetNameCore() const
    {
        // fallback to title if profile name is empty
        if (auto control{ _termControl.get() })
        {
            const auto profileName = control->GetProfileName();
            if (profileName.empty())
            {
                return control->Title();
            }
            else
            {
                return profileName;
            }
        }

        return L"";
    }

    hstring TermControlAutomationPeer::GetHelpTextCore() const
    {
        if (const auto control{ _termControl.get() })
        {
            return control->Title();
        }
        return L"";
    }

    AutomationLiveSetting TermControlAutomationPeer::GetLiveSettingCore() const
    {
        return AutomationLiveSetting::Polite;
Repository: microsoft/terminal
File: src/cascadia/WindowsTerminal_UIATests/Elements/TerminalApp.cs (C#)

Code before:

            UIRoot = Session.FindElementByName(WindowTitleToFind);

            // Set the timeout to 15 seconds after we found the initial window.
            Session.Manage().Timeouts().ImplicitWait = TimeSpan.FromSeconds(15);
        }

        private bool IsRunningAsAdmin()
        {
            return new WindowsPrincipal(WindowsIdentity.GetCurrent()).IsInRole(WindowsBuiltInRole.Administrator);
        }

        private void ExitProcess()
        {
            Globals.SweepAllModules(this.context);

            // Release attachment to the child process console.
            WinCon.FreeConsole();

            this.UIRoot = null;

            if (this.job == IntPtr.Zero)
            {
                return;
            }
            WinBase.TerminateJobObject(this.job, 0);
        }
    }
}


Review comment (suggestion):
This isn't equivalent due to the lack of `this.job = ...`. I'd suggest:
```suggestion
            if (this.job != IntPtr.Zero)
            {
                WinBase.TerminateJobObject(this.job, 0);
                this.job = IntPtr.Zero;
            }
```

Code after:

            UIRoot = Session.FindElementByName(WindowTitleToFind);

            // Set the timeout to 15 seconds after we found the initial window.
            Session.Manage().Timeouts().ImplicitWait = TimeSpan.FromSeconds(15);
        }

        private bool IsRunningAsAdmin()
        {
            return new WindowsPrincipal(WindowsIdentity.GetCurrent()).IsInRole(WindowsBuiltInRole.Administrator);
        }

        private void ExitProcess()
        {
            Globals.SweepAllModules(this.context);

            // Release attachment to the child process console.
            WinCon.FreeConsole();

            this.UIRoot = null;

            if (this.job != IntPtr.Zero)
            {
                WinBase.TerminateJobObject(this.job, 0);
                this.job = IntPtr.Zero;
            }
        }
    }
}
Repository: microsoft/terminal
File: src/host/ft_uia/Elements/CmdApp.cs (C#)

Code before:
                    {
                        this.CreateCmdProcess(binPath, pathOverride);
                    }
                    else
                    {
                        this.CreateCmdProcess(binPath, linkPath);
                    }
                    break;
                default:
                    throw new NotImplementedException(AutoHelpers.FormatInvariant("CreateType '{0}' not implemented.", type.ToString()));
            }
        }

        private void ExitCmdProcess()
        {
            // Release attachment to the child process console.
            WinCon.FreeConsole();

            this.UIRoot = null;

            if (this.job == IntPtr.Zero)
            {
                return;
            }
            WinBase.TerminateJobObject(this.job, 0);
            this.job = IntPtr.Zero;
        }

        public WinEventSystem AttachWinEventSystem(IWinEventCallbacks callbacks)
        {
            return new WinEventSystem(callbacks, (uint)this.pid);
        }
    }
}


Review comment (suggestion):
Same here. I feel like this is easier to read:
```suggestion
            if (this.job != IntPtr.Zero)
            {
                WinBase.TerminateJobObject(this.job, 0);
                this.job = IntPtr.Zero;
            }
```

Code after:
                    {
                        this.CreateCmdProcess(binPath, pathOverride);
                    }
                    else
                    {
                        this.CreateCmdProcess(binPath, linkPath);
                    }
                    break;
                default:
                    throw new NotImplementedException(AutoHelpers.FormatInvariant("CreateType '{0}' not implemented.", type.ToString()));
            }
        }

        private void ExitCmdProcess()
        {
            // Release attachment to the child process console.
            WinCon.FreeConsole();

            this.UIRoot = null;

            if (this.job != IntPtr.Zero)
            {
                WinBase.TerminateJobObject(this.job, 0);
                this.job = IntPtr.Zero;
            }
        }

        public WinEventSystem AttachWinEventSystem(IWinEventCallbacks callbacks)
        {
            return new WinEventSystem(callbacks, (uint)this.pid);
        }
    }
}