ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/PowerToys
File: src/modules/fancyzones/FancyZonesLib/MonitorWorkAreaMap.cpp (C++)

Code before:
    }
}

const std::unordered_map<HMONITOR, std::unique_ptr<WorkArea>>& MonitorWorkAreaMap::GetAllWorkAreas() const noexcept
{
    return m_workAreaMap;
}

void MonitorWorkAreaMap::AddWorkArea(HMONITOR monitor, std::unique_ptr<WorkArea> workArea)
{
    m_workAreaMap.insert({ monitor, std::move(workArea) });
}

FancyZonesDataTypes::WorkAreaId MonitorWorkAreaMap::GetParent(HMONITOR monitor) const
{
    if (m_workAreaParents.contains(monitor))
    {
        return m_workAreaParents.at(monitor);
    }
    
    return FancyZonesDataTypes::WorkAreaId{};
}

void MonitorWorkAreaMap::Clear() noexcept
{
    // save parent ids
    m_workAreaParents.clear();
    for (const auto& [monitor, workArea] : m_workAreaMap)
    {
        if (workArea)
        {
            m_workAreaParents.insert({ monitor, workArea->UniqueId() });
        }
    }

    m_workAreaMap.clear();
}


Review comment (refactor):
change looks good, but I'd prefer this out of Clear() function as it's a bit odd having it there. Maybe  extract to separate function and call in `UpdateWorkAreas` (and if needed elsewhere)

Code after:
    }
}

const std::unordered_map<HMONITOR, std::unique_ptr<WorkArea>>& MonitorWorkAreaMap::GetAllWorkAreas() const noexcept
{
    return m_workAreaMap;
}

void MonitorWorkAreaMap::AddWorkArea(HMONITOR monitor, std::unique_ptr<WorkArea> workArea)
{
    m_workAreaMap.insert({ monitor, std::move(workArea) });
}

FancyZonesDataTypes::WorkAreaId MonitorWorkAreaMap::GetParent(HMONITOR monitor) const
{
    if (m_workAreaParents.contains(monitor))
    {
        return m_workAreaParents.at(monitor);
    }
    
    return FancyZonesDataTypes::WorkAreaId{};
}

void MonitorWorkAreaMap::SaveParentIds()
{
    m_workAreaParents.clear();
    for (const auto& [monitor, workArea] : m_workAreaMap)
    {
        if (workArea)
        {
            m_workAreaParents.insert({ monitor, workArea->UniqueId() });
        }
    }
}

void MonitorWorkAreaMap::Clear() noexcept
{
    m_workAreaMap.clear();
}
Repository: microsoft/PowerToys
File: src/settings-ui/Settings.UI/ViewModels/GeneralViewModel.cs (C#)

Code before:
            get
            {
                return _runElevated;
            }

            set
            {
                if (_runElevated != value)
                {
                    _runElevated = value;
                    GeneralSettingsConfig.RunElevated = value;
                    NotifyPropertyChanged();
                }
            }
        }

        // Gets a value indicating whether the user is part of administrators group.
        public bool IsAdmin
        {
            get
            {
                return _isAdmin;
            }
        }

        // Are we running a dev build? (Please note that we verify this in the code that gets the newest version from Github too.)
        public static bool AutoUpdatesDisabledOnDevBuild
        {
            get
            {
                return Helper.GetProductVersion() == "v0.0.1";
            }
        }

        public bool AutoDownloadUpdates
        {
            get
            {
                return _autoDownloadUpdates && !_autoDownloadUpdatesIsGpoDisabled;
            }

            set
            {
                if (_autoDownloadUpdates != value)
                {
                    _autoDownloadUpdates = value;
                    GeneralSettingsConfig.AutoDownloadUpdates = value;
                    NotifyPropertyChanged();
                }
            }
        }

Review comment (suggestion):
![image](https://user-images.githubusercontent.com/26118718/221191932-b458157f-cb3f-49f0-8cff-b88307ef88b7.png)

I'll update this one.

Code after:
            get
            {
                return _runElevated;
            }

            set
            {
                if (_runElevated != value)
                {
                    _runElevated = value;
                    GeneralSettingsConfig.RunElevated = value;
                    NotifyPropertyChanged();
                }
            }
        }

        // Gets a value indicating whether the user is part of administrators group.
        public bool IsAdmin
        {
            get
            {
                return _isAdmin;
            }
        }

        // Are we running a dev build? (Please note that we verify this in the code that gets the newest version from GitHub too.)
        public static bool AutoUpdatesDisabledOnDevBuild
        {
            get
            {
                return Helper.GetProductVersion() == "v0.0.1";
            }
        }

        public bool AutoDownloadUpdates
        {
            get
            {
                return _autoDownloadUpdates && !_autoDownloadUpdatesIsGpoDisabled;
            }

            set
            {
                if (_autoDownloadUpdates != value)
                {
                    _autoDownloadUpdates = value;
                    GeneralSettingsConfig.AutoDownloadUpdates = value;
                    NotifyPropertyChanged();
                }
            }
        }
Repository: microsoft/PowerToys
File: src/modules/powerrename/PowerRenameUILib/MainWindow.xaml.cpp (C++)

Code before:
        if (SUCCEEDED(m_prManager->GetItemById(id, &spItem)))
        {
            spItem->PutSelected(checked);
        }
        UpdateCounts();
    }

    void MainWindow::ToggleAll()
    {
        _TRACER_;

        UINT itemCount = 0;
        m_prManager->GetItemCount(&itemCount);
        bool selected = checkBox_selectAll().IsChecked().GetBoolean();
        for (UINT i = 0; i < itemCount; i++)
        {
            CComPtr<IPowerRenameItem> spItem;
            if (SUCCEEDED(m_prManager->GetItemByIndex(i, &spItem)))
            {
                spItem->PutSelected(selected);
                int id = 0;
                spItem->GetId(&id);
                auto item = FindById(id);
                if (item)
                    item.Checked(selected);
            }
        }
        UpdateCounts();
    }

    void MainWindow::SwitchView()
    {
        _TRACER_;

        m_prManager->SwitchFilter(0);
        PopulateExplorerItems();
        UpdateCounts();
    }

    void MainWindow::Rename(bool closeWindow)
    {
        _TRACER_;

        if (m_prManager)
        {
            m_prManager->Rename(m_window, closeWindow);
        }

        // Persist the current settings.  We only do this when
        // a rename is actually performed.  Not when the user
        // closes/cancels the dialog.

Review comment (suggestion):
even better, instead of iterating here over all elements (GetItemCount), you can iterate over m_explorerItemsMap (or m_explorerItems), and check/unchech those items, as these collections are updated on ShowAll/ShowRenamed.


Code after:
    {
        _TRACER_;
        Logger::debug(L"Toggling item with id = {}", id);
        CComPtr<IPowerRenameItem> spItem;

        if (SUCCEEDED(m_prManager->GetItemById(id, &spItem)))
        {
            spItem->PutSelected(checked);
        }
        UpdateCounts();
    }

    void MainWindow::ToggleAll()
    {
        _TRACER_;

        UINT itemCount = 0;
        m_prManager->GetItemCount(&itemCount);
        bool selected = checkBox_selectAll().IsChecked().GetBoolean();
        for (UINT i = 0; i < itemCount; i++)
        {
            CComPtr<IPowerRenameItem> spItem;
            if (SUCCEEDED(m_prManager->GetItemByIndex(i, &spItem)))
            {
                spItem->PutSelected(selected);
                int id = 0;
                spItem->GetId(&id);
                auto item = FindById(id);
                if (item)
                    item.Checked(selected);
            }
        }
        UpdateCounts();
    }

    void MainWindow::SwitchView()
    {
        _TRACER_;

        m_prManager->SwitchFilter(0);
        PopulateExplorerItems();
        UpdateCounts();
    }

    void MainWindow::Rename(bool closeWindow)
    {
        _TRACER_;

        if (m_prManager)
        {
            m_prManager->Rename(m_window, closeWindow);
Repository: microsoft/PowerToys
File: src/modules/videoconference/VideoConferenceModule/VideoConferenceModule.cpp (C++)

Code before:

#include <WinUser.h>

#include <gdiplus.h>
#include <shellapi.h>

#include <filesystem>
#include <unordered_set>

#include <common/debug_control.h>
#include <common/SettingsAPI/settings_helpers.h>
#include <common/utils/elevation.h>
#include <common/utils/process_path.h>

#include <CameraStateUpdateChannels.h>

#include "logging.h"
#include "trace.h"

extern "C" IMAGE_DOS_HEADER __ImageBase;

VideoConferenceModule* instance = nullptr;

VideoConferenceSettings VideoConferenceModule::settings;
Toolbar VideoConferenceModule::toolbar;
bool VideoConferenceModule::pushToTalkPressed;

HHOOK VideoConferenceModule::hook_handle;

IAudioEndpointVolume* endpointVolume = NULL;

bool VideoConferenceModule::isKeyPressed(unsigned int keyCode)
{
    return (GetKeyState(keyCode) & 0x8000);
}

namespace fs = std::filesystem;

bool VideoConferenceModule::isHotkeyPressed(DWORD code, PowerToysSettings::HotkeyObject& hotkey)
{
    return code == hotkey.get_code() &&
           isKeyPressed(VK_SHIFT) == hotkey.shift_pressed() &&
           isKeyPressed(VK_CONTROL) == hotkey.ctrl_pressed() &&
           isKeyPressed(VK_LWIN) == hotkey.win_pressed() &&
           (isKeyPressed(VK_LMENU)) == hotkey.alt_pressed();
}

void VideoConferenceModule::reverseMicrophoneMute()
{
    bool muted = false;
    for (auto& controlledMic : instance->_controlledMicrophones)

Review comment (suggestion):
Even though it's declared as `static`, let's add it for clarity.

```suggestion
bool VideoConferenceModule::pushToTalkPressed = false;
```

Code after:

#include <WinUser.h>

#include <gdiplus.h>
#include <shellapi.h>

#include <filesystem>
#include <unordered_set>

#include <common/debug_control.h>
#include <common/SettingsAPI/settings_helpers.h>
#include <common/utils/elevation.h>
#include <common/utils/process_path.h>

#include <CameraStateUpdateChannels.h>

#include "logging.h"
#include "trace.h"

extern "C" IMAGE_DOS_HEADER __ImageBase;

VideoConferenceModule* instance = nullptr;

VideoConferenceSettings VideoConferenceModule::settings;
Toolbar VideoConferenceModule::toolbar;
bool VideoConferenceModule::pushToTalkPressed = false;

HHOOK VideoConferenceModule::hook_handle;

IAudioEndpointVolume* endpointVolume = NULL;

bool VideoConferenceModule::isKeyPressed(unsigned int keyCode)
{
    return (GetKeyState(keyCode) & 0x8000);
}

namespace fs = std::filesystem;

bool VideoConferenceModule::isHotkeyPressed(DWORD code, PowerToysSettings::HotkeyObject& hotkey)
{
    return code == hotkey.get_code() &&
           isKeyPressed(VK_SHIFT) == hotkey.shift_pressed() &&
           isKeyPressed(VK_CONTROL) == hotkey.ctrl_pressed() &&
           isKeyPressed(VK_LWIN) == hotkey.win_pressed() &&
           (isKeyPressed(VK_LMENU)) == hotkey.alt_pressed();
}

void VideoConferenceModule::reverseMicrophoneMute()
{
    bool muted = false;
    for (auto& controlledMic : instance->_controlledMicrophones)
Repository: microsoft/PowerToys
File: src/runner/UpdateUtils.cpp (C++)

Code before:

void CheckForUpdatesCallback()
{
    Logger::trace(L"Check for updates callback invoked");
    auto state = UpdateState::read();
    try
    {
        auto new_version_info = get_github_version_info_async().get();
        if (!new_version_info)
        {
            // If we couldn't get a new version from github for some reason, assume we're up to date, but also log error
            new_version_info = version_up_to_date{};
            Logger::error(L"Couldn't obtain version info from github: {}", new_version_info.error());
        }

        // Auto download setting
        bool download_update = !IsMeteredConnection() && get_general_settings().downloadUpdatesAutomatically;
        if (powertoys_gpo::getDisableAutomaticUpdateDownloadValue() == powertoys_gpo::gpo_rule_configured_enabled)
        {
            Logger::info(L"Automatic download of updates is disabled by GPO.");
            download_update = false;
        }
        
        ProcessNewVersionInfo(*new_version_info, state, download_update, false);

        std::lock_guard<std::mutex> lock(state_mutex);
        UpdateState::store([&](UpdateState& v) {
            v = std::move(state);
        });
    }
    catch (...)
    {
        Logger::error("CheckForUpdatesCallback: error while processing version info");
    }
}


Review comment (suggestion):
`UpdateState::store` already uses mutex internally, why do we need another one? 🤔 

Code after:
{
    Logger::trace(L"Check for updates callback invoked");
    auto state = UpdateState::read();
    try
    {
        auto new_version_info = get_github_version_info_async().get();
        if (!new_version_info)
        {
            // If we couldn't get a new version from github for some reason, assume we're up to date, but also log error
            new_version_info = version_up_to_date{};
            Logger::error(L"Couldn't obtain version info from github: {}", new_version_info.error());
        }

        // Auto download setting
        bool download_update = !IsMeteredConnection() && get_general_settings().downloadUpdatesAutomatically;
        if (powertoys_gpo::getDisableAutomaticUpdateDownloadValue() == powertoys_gpo::gpo_rule_configured_enabled)
        {
            Logger::info(L"Automatic download of updates is disabled by GPO.");
            download_update = false;
        }
        
        ProcessNewVersionInfo(*new_version_info, state, download_update, false);

        UpdateState::store([&](UpdateState& v) {
            v = std::move(state);
        });
    }
    catch (...)
    {
        Logger::error("CheckForUpdatesCallback: error while processing version info");
    }
}
Repository: microsoft/PowerToys
File: src/modules/fancyzones/FancyZonesLib/DraggingState.h (C/C++)

Code before:

#include <FancyZonesLib/KeyState.h>
#include <FancyZonesLib/MiddleMouseButtonsHook.h>
#include <FancyZonesLib/SecondaryMouseButtonsHook.h>

class DraggingState
{
public:
    DraggingState(const std::function<void()>& keyUpdateCallback);
    ~DraggingState() = default;

    void Enable();
    void Disable();
    void UpdateDraggingState() noexcept;

    bool IsDragging() const noexcept;
    bool IsSelectManyZonesState() const noexcept;

private:
    void OnMouseDown();
    void OnMiddleMouseDown();

    std::atomic<bool> m_mouseState;
    std::atomic<bool> m_middleMouseState;
    SecondaryMouseButtonsHook m_mouseHook;
    MiddleMouseButtonsHook m_middleMouseHook;
    KeyState<VK_LSHIFT> m_leftShiftKeyState;
    KeyState<VK_RSHIFT> m_rightShiftKeyState;
    KeyState<VK_LCONTROL, VK_RCONTROL> m_ctrlKeyState;
    std::function<void()> m_keyUpdateCallback;

    bool m_dragging{}; // True if we should be showing zone hints while dragging
};


Review comment (suggestion):
I believe it'd be better to make `SecondaryMouseButtonHook` just `MouseButtonHook` and add there different callbacks for right-click and middle-click.

Code after:

#include <FancyZonesLib/KeyState.h>
#include <FancyZonesLib/MouseButtonsHook.h>

class DraggingState
{
public:
    DraggingState(const std::function<void()>& keyUpdateCallback);
    ~DraggingState() = default;

    void Enable();
    void Disable();
    void UpdateDraggingState() noexcept;

    bool IsDragging() const noexcept;
    bool IsSelectManyZonesState() const noexcept;

private:
    void OnSecondaryMouseDown();
    void OnMiddleMouseDown();

    std::atomic<bool> m_secondaryMouseState;
    std::atomic<bool> m_middleMouseState;
    MouseButtonsHook m_mouseHook;
    KeyState<VK_LSHIFT> m_leftShiftKeyState;
    KeyState<VK_RSHIFT> m_rightShiftKeyState;
    KeyState<VK_LCONTROL, VK_RCONTROL> m_ctrlKeyState;
    std::function<void()> m_keyUpdateCallback;

    bool m_dragging{}; // True if we should be showing zone hints while dragging
};
Repository: microsoft/PowerToys
File: src/modules/fancyzones/FancyZonesLib/DraggingState.cpp (C++)

Code before:
}

void DraggingState::UpdateDraggingState() noexcept
{
    // This updates m_dragEnabled depending on if the shift key is being held down
    if (FancyZonesSettings::settings().shiftDrag)
    {
        m_dragging = ((m_leftShiftKeyState.state() || m_rightShiftKeyState.state()) ^ m_mouseState);
    }
    else
    {
        m_dragging = !((m_leftShiftKeyState.state() || m_rightShiftKeyState.state()) ^ m_mouseState);
    }
}

void DraggingState::OnMouseDown()
{
    m_mouseState = !m_mouseState;
    m_keyUpdateCallback();
}

void DraggingState::OnMiddleMouseDown()
{
    m_middleMouseState = !m_middleMouseState;
    m_keyUpdateCallback();
}

bool DraggingState::IsDragging() const noexcept
{
    return m_dragging;
}

bool DraggingState::IsSelectManyZonesState() const noexcept
{
    return m_ctrlKeyState.state() ^ m_middleMouseState;
}

Review comment (suggestion):
Let's update the state only when we're in the correct mode. I think it can be confusing when you just initiated dragging by right click and it's already in the zone-merging mode (because of clicking the middle mouse before dragging).

Code after:
{
    // This updates m_dragEnabled depending on if the shift key is being held down
    if (FancyZonesSettings::settings().shiftDrag)
    {
        m_dragging = ((m_leftShiftKeyState.state() || m_rightShiftKeyState.state()) ^ m_secondaryMouseState);
    }
    else
    {
        m_dragging = !((m_leftShiftKeyState.state() || m_rightShiftKeyState.state()) ^ m_secondaryMouseState);
    }
}

void DraggingState::OnSecondaryMouseDown()
{
    m_secondaryMouseState = !m_secondaryMouseState;
    m_keyUpdateCallback();
}

void DraggingState::OnMiddleMouseDown()
{
    if (!this->IsDragging())
    {
        return;
    }

    m_middleMouseState = !m_middleMouseState;
    m_keyUpdateCallback();
}

bool DraggingState::IsDragging() const noexcept
{
    return m_dragging;
}

bool DraggingState::IsSelectManyZonesState() const noexcept
{
    return m_ctrlKeyState.state() || m_middleMouseState;
}
Repository: microsoft/PowerToys
File: src/modules/fancyzones/FancyZonesLib/DraggingState.h (C/C++)

Code before:
#pragma once

#include <FancyZonesLib/KeyState.h>
#include <FancyZonesLib/MouseButtonsHook.h>

class DraggingState
{
public:
    DraggingState(const std::function<void()>& keyUpdateCallback);
    ~DraggingState() = default;

    void Enable();
    void Disable();
    void UpdateDraggingState() noexcept;

    bool IsDragging() const noexcept;
    bool IsSelectManyZonesState() const noexcept;

private:
    void OnMouseDown();
    void OnMiddleMouseDown();

    std::atomic<bool> m_mouseState;
    std::atomic<bool> m_middleMouseState;
    MouseButtonsHook m_mouseHook;
    KeyState<VK_LSHIFT> m_leftShiftKeyState;
    KeyState<VK_RSHIFT> m_rightShiftKeyState;
    KeyState<VK_LCONTROL, VK_RCONTROL> m_ctrlKeyState;
    std::function<void()> m_keyUpdateCallback;

    bool m_dragging{}; // True if we should be showing zone hints while dragging
};


Review comment (suggestion):
Same thing, let's clarify that it's `OnSecondaryMouseDown`

Code after:
#pragma once

#include <FancyZonesLib/KeyState.h>
#include <FancyZonesLib/MouseButtonsHook.h>

class DraggingState
{
public:
    DraggingState(const std::function<void()>& keyUpdateCallback);
    ~DraggingState() = default;

    void Enable();
    void Disable();
    void UpdateDraggingState() noexcept;

    bool IsDragging() const noexcept;
    bool IsSelectManyZonesState() const noexcept;

private:
    void OnSecondaryMouseDown();
    void OnMiddleMouseDown();

    std::atomic<bool> m_secondaryMouseState;
    std::atomic<bool> m_middleMouseState;
    MouseButtonsHook m_mouseHook;
    KeyState<VK_LSHIFT> m_leftShiftKeyState;
    KeyState<VK_RSHIFT> m_rightShiftKeyState;
    KeyState<VK_LCONTROL, VK_RCONTROL> m_ctrlKeyState;
    std::function<void()> m_keyUpdateCallback;

    bool m_dragging{}; // True if we should be showing zone hints while dragging
};
Repository: microsoft/PowerToys
File: src/modules/previewpane/MarkdownPreviewHandler/Settings.cs (C#)

Code before:
﻿// Copyright (c) Microsoft Corporation
// The Microsoft Corporation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Microsoft.PowerToys.PreviewHandler.Markdown
{
    internal sealed class Settings
    {
        /// <summary>
        /// Gets the color of the window background.
        /// </summary>
        public static Color BackgroundColor
        {
            get
            {
                if (GetTheme() == "dark")
                {
                    return Color.FromArgb(30, 30, 30); // #1e1e1e
                }
                else
                {
                    return Color.White;
                }
            }
        }

        /// <summary>
        /// Returns the theme.
        /// </summary>
        /// <returns>Theme that should be used.</returns>
        public static string GetTheme()
        {
            return Common.UI.ThemeManager.GetWindowsBaseColor().ToLowerInvariant();
        }
    }

Review comment (suggestion):
```suggestion
        /// Gets the color of the window background.
        /// Even though this is not a setting yet, it's retrieved from a "Settings" class to be aligned with other preview handlers that contain this setting.
        /// It's possible it can be converted into a setting in the future.
```

Code after:
﻿// Copyright (c) Microsoft Corporation
// The Microsoft Corporation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Microsoft.PowerToys.PreviewHandler.Markdown
{
    internal sealed class Settings
    {
        /// <summary>
        /// Gets the color of the window background.
        /// Even though this is not a setting yet, it's retrieved from a "Settings" class to be aligned with other preview handlers that contain this setting.
        /// It's possible it can be converted into a setting in the future.
        /// </summary>
        public static Color BackgroundColor
        {
            get
            {
                if (GetTheme() == "dark")
                {
                    return Color.FromArgb(30, 30, 30); // #1e1e1e
                }
                else
                {
                    return Color.White;
                }
            }
        }

        /// <summary>
        /// Returns the theme.
        /// </summary>
        /// <returns>Theme that should be used.</returns>
        public static string GetTheme()
        {
            return Common.UI.ThemeManager.GetWindowsBaseColor().ToLowerInvariant();