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

Code before:
        else
        {
            mouse.dwFlags = RIDEV_REMOVE;
            mouse.hwndTarget = nullptr;
        }
        RegisterRawInputDevices(&mouse, 1, sizeof(mouse));
    }
}

template<typename D>
bool SuperSonar<D>::IsForegroundAppExcluded()
{
    if (m_excludedApps.size() < 1)
    {
        return false;
    }
    if (HWND foregroundApp{ GetForegroundWindow() })
    {
        auto processPath = get_process_path(foregroundApp);
        CharUpperBuffW(processPath.data(), static_cast<DWORD>(processPath.length()));
        bool res = find_app_name_in_path(processPath, m_excludedApps);

        if (res == false)
        {
            res = check_excluded_app_with_title(foregroundApp, processPath, m_excludedApps);
        }

        return res;
    }
    else
    {
        return false;
    }
}

struct CompositionSpotlight : SuperSonar<CompositionSpotlight>
{
    static constexpr UINT WM_OPACITY_ANIMATION_COMPLETED = WM_APP;
    float m_sonarRadiusFloat = static_cast<float>(m_sonarRadius);

    DWORD GetExtendedStyle()
    {
        return WS_EX_NOREDIRECTIONBITMAP;
    }

    void AfterMoveSonar()
    {
        m_spotlight.Offset({ static_cast<float>(m_sonarPos.x), static_cast<float>(m_sonarPos.y), 0.0f });
    }

    LRESULT WndProc(UINT message, WPARAM wParam, LPARAM lParam) noexcept

Review comment (suggestion):
I believe it makes sense to create a function that calls `find_app_name_in_path` and `check_excluded_app_with_title`, as everywhere they're called together. It will help to keep the code clean and avoid copy-pasting. 

Code after:
        else
        {
            mouse.dwFlags = RIDEV_REMOVE;
            mouse.hwndTarget = nullptr;
        }
        RegisterRawInputDevices(&mouse, 1, sizeof(mouse));
    }
}

template<typename D>
bool SuperSonar<D>::IsForegroundAppExcluded()
{
    if (m_excludedApps.size() < 1)
    {
        return false;
    }
    if (HWND foregroundApp{ GetForegroundWindow() })
    {
        auto processPath = get_process_path(foregroundApp);
        CharUpperBuffW(processPath.data(), static_cast<DWORD>(processPath.length()));

        return check_excluded_app(foregroundApp, processPath, m_excludedApps);
    }
    else
    {
        return false;
    }
}

struct CompositionSpotlight : SuperSonar<CompositionSpotlight>
{
    static constexpr UINT WM_OPACITY_ANIMATION_COMPLETED = WM_APP;
    float m_sonarRadiusFloat = static_cast<float>(m_sonarRadius);

    DWORD GetExtendedStyle()
    {
        return WS_EX_NOREDIRECTIONBITMAP;
    }

    void AfterMoveSonar()
    {
        m_spotlight.Offset({ static_cast<float>(m_sonarPos.x), static_cast<float>(m_sonarPos.y), 0.0f });
    }

    LRESULT WndProc(UINT message, WPARAM wParam, LPARAM lParam) noexcept
    {
        switch (message)
        {
        case WM_CREATE:
            return OnCompositionCreate() && BaseWndProc(message, wParam, lParam);
Repository: microsoft/PowerToys
File: src/modules/launcher/Wox.Plugin/IPlugin.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.Collections.Generic;

namespace Wox.Plugin
{
    public interface IPlugin
    {
        List<Result> Query(Query query);

        void Init(PluginInitContext context);

        // Localized name
        string Name { get; }

        // Localized description
        string Description { get; }

        // Plugin ID for validating the plugin.json entry (It must be static for accessing it before loading the plugin.)
        public static string PluginID { get; }
    }
}


Review comment (refactor):
This isn't working as expected. I don't get any warning if the property is missing wen building the plugin code.

Maybe caused by this: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-11.0/static-abstracts-in-interfaces

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;

namespace Wox.Plugin
{
    public interface IPlugin
    {
        List<Result> Query(Query query);

        void Init(PluginInitContext context);

        // Localized name
        string Name { get; }

        // Localized description
        string Description { get; }

        /* The two property lines are commented because they break the unit tests. (The Moq package used in the unit tests doesn't support the .Net 7 feature 'static abstract' properties yet.) - https://github.com/Moq/Moq/issues/1398
        *
        * // Plugin ID for validating the plugin.json entry (It must be static for accessing it before loading the plugin.)
        * public static abstract string PluginID { get; }
        */
    }
}
Repository: microsoft/PowerToys
File: src/modules/launcher/Wox.Plugin/IPlugin.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.Collections.Generic;

namespace Wox.Plugin
{
    public interface IPlugin
    {
        List<Result> Query(Query query);

        void Init(PluginInitContext context);

        // Localized name
        string Name { get; }

        // Localized description
        string Description { get; }

        // Plugin ID for validating the plugin.json entry (It must be static for accessing it before loading the plugin.)
        public static abstract string PluginID { get; }
    }
}


Review comment (bug):
@crutkas , @jaimecbernardo
The new .Net 7 feature `static abstract` interface properties are not supported by the Moq framework. This causes the tests to fail. (https://github.com/moq/moq/issues/1398)

What should we do? There are three options I think:
1. Making it an interface member and throw `NotImplemented` when called as interface property. (Then you can compile plugins without having the property.)
2. Removing the affected test cases.
3. Splitting the interface into two nested interfaces and updating the test code to use the base interface (IPluginBase). *

*) Example
```
interface IPlugin : IPuginBase
{
  public static abstract  string PluginID { get; }
}

interface IPluginBase
{
  ...
  string Name { get; }
  string Description { get; }
}
```

I prefer option 3.

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;

namespace Wox.Plugin
{
    public interface IPlugin
    {
        List<Result> Query(Query query);

        void Init(PluginInitContext context);

        // Localized name
        string Name { get; }

        // Localized description
        string Description { get; }

        /* The two property lines are commented because they break the unit tests. (The Moq package used in the unit tests doesn't support the .Net 7 feature 'static abstract' properties yet.) - https://github.com/Moq/Moq/issues/1398
        *
        * // Plugin ID for validating the plugin.json entry (It must be static for accessing it before loading the plugin.)
        * public static abstract string PluginID { get; }
        */
    }
}
Repository: microsoft/PowerToys
File: src/modules/launcher/Wox.Plugin/IPlugin.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;

namespace Wox.Plugin
{
    public interface IPlugin
    {
        List<Result> Query(Query query);

        void Init(PluginInitContext context);

        // Localized name
        string Name { get; }

        // Localized description
        string Description { get; }

        /* The two property lines are commented because they brake the unit tests. (The Moq package used in the unit tests doesn't support the .Net 7 feature 'static abstract' properties yet.) - https://github.com/Moq/Moq/issues/1398
        *
        * // Plugin ID for validating the plugin.json entry (It must be static for accessing it before loading the plugin.)
        * public static abstract string PluginID { get; }
        */
    }
}


Review comment (refactor):
```suggestion
        /* The two property lines are commented because they break the unit tests. (The Moq package used in the unit tests doesn't support the .Net 7 feature 'static abstract' properties yet.) - https://github.com/Moq/Moq/issues/1398
```

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;

namespace Wox.Plugin
{
    public interface IPlugin
    {
        List<Result> Query(Query query);

        void Init(PluginInitContext context);

        // Localized name
        string Name { get; }

        // Localized description
        string Description { get; }

        /* The two property lines are commented because they break the unit tests. (The Moq package used in the unit tests doesn't support the .Net 7 feature 'static abstract' properties yet.) - https://github.com/Moq/Moq/issues/1398
        *
        * // Plugin ID for validating the plugin.json entry (It must be static for accessing it before loading the plugin.)
        * public static abstract string PluginID { get; }
        */
    }
}
Repository: microsoft/PowerToys
File: src/modules/launcher/Plugins/Microsoft.Plugin.Shell/Main.cs (C#)

Code before:
                    Title = Properties.Resources.wox_plugin_cmd_run_as_user,
                    Glyph = "\xE7EE",
                    FontFamily = "Segoe MDL2 Assets",
                    AcceleratorKey = Key.U,
                    AcceleratorModifiers = ModifierKeys.Control | ModifierKeys.Shift,
                    Action = _ =>
                    {
                        Execute(Process.Start, PrepareProcessStartInfo(selectedResult.Title, RunAsType.OtherUser));
                        return true;
                    },
                },
            };

            return resultlist;
        }

        public void UpdateSettings(PowerLauncherPluginSettings settings)
        {
            var leaveShellOpen = false;

            if (settings != null && settings.AdditionalOptions != null)
            {
                var optionConfirm = settings.AdditionalOptions.FirstOrDefault(x => x.Key == "LeaveShellOpen");
                leaveShellOpen = optionConfirm?.Value ?? leaveShellOpen;
                _settings.LeaveShellOpen = leaveShellOpen;
            }

            Save();
        }
    }
}


Review comment (suggestion):
```suggestion
                var optionLeaveShellOpen = settings.AdditionalOptions.FirstOrDefault(x => x.Key == "LeaveShellOpen");
                leaveShellOpen = optionLeaveShellOpen?.Value ?? leaveShellOpen;
                _settings.LeaveShellOpen = leaveShellOpen;
            }
```

Code after:
                    Title = Properties.Resources.wox_plugin_cmd_run_as_user,
                    Glyph = "\xE7EE",
                    FontFamily = "Segoe MDL2 Assets",
                    AcceleratorKey = Key.U,
                    AcceleratorModifiers = ModifierKeys.Control | ModifierKeys.Shift,
                    Action = _ =>
                    {
                        Execute(Process.Start, PrepareProcessStartInfo(selectedResult.Title, RunAsType.OtherUser));
                        return true;
                    },
                },
            };

            return resultlist;
        }

        public void UpdateSettings(PowerLauncherPluginSettings settings)
        {
            var leaveShellOpen = false;

            if (settings != null && settings.AdditionalOptions != null)
            {
                var optionLeaveShellOpen = settings.AdditionalOptions.FirstOrDefault(x => x.Key == "LeaveShellOpen");
                leaveShellOpen = optionLeaveShellOpen?.Value ?? leaveShellOpen;
                _settings.LeaveShellOpen = leaveShellOpen;
            }

            Save();
        }
    }
}
Repository: microsoft/PowerToys
File: src/settings-ui/Settings.UI/ViewModels/PluginAdditionalOptionViewModel.cs (C#)

Code before:
                if (value != _additionalOption.Value)
                {
                    _additionalOption.Value = value;
                    NotifyPropertyChanged();
                }
            }
        }

        public List<string> ComboBoxOptions => _additionalOption.ComboBoxOptions;

        public int Option
        {
            get => _additionalOption.Option;
            set
            {
                if (value != _additionalOption.Option)
                {
                    _additionalOption.Option = value;
                    NotifyPropertyChanged();
                }
            }
        }

        public bool ShowComboBox => _additionalOption.ComboBoxOptions != null && _additionalOption.ComboBoxOptions.Count > 0;

        public bool ShowCheckBox => ShowComboBox == false;

        public event PropertyChangedEventHandler PropertyChanged;

        private void NotifyPropertyChanged([CallerMemberName] string propertyName = "")
        {
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
        }
    }
}


Review comment (suggestion):
Can we make this more flexible by supporting checkbox value (`Value`) being not initialized (`null`). Then we can show the checkboox if `Value` is true or false.

This allows us to have three different settings combinations: DropDown, Checkbox, Checkbox&DropDown

Code after:
                {
                    _additionalOption.Value = value;
                    NotifyPropertyChanged();
                }
            }
        }

        public List<string> ComboBoxOptions => _additionalOption.ComboBoxOptions;

        public int Option
        {
            get => _additionalOption.Option;
            set
            {
                if (value != _additionalOption.Option)
                {
                    _additionalOption.Option = value;
                    NotifyPropertyChanged();
                }
            }
        }

        public bool ShowComboBox => _additionalOption.SelectionTypeValue == (int)PluginAdditionalOption.SelectionType.Combobox && _additionalOption.ComboBoxOptions != null && _additionalOption.ComboBoxOptions.Count > 0;

        public bool ShowCheckBox => _additionalOption.SelectionTypeValue == (int)PluginAdditionalOption.SelectionType.Checkbox;

        public event PropertyChangedEventHandler PropertyChanged;

        private void NotifyPropertyChanged([CallerMemberName] string propertyName = "")
        {
            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
        }
    }
}
Repository: microsoft/PowerToys
File: src/modules/fancyzones/UnitTests-FancyZonesEditor/GridLayoutModelTests.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 FancyZonesEditor.Models;

namespace UnitTestFancyZonesEditor;

[TestClass]
public class GridLayoutModelTests
{
    [TestMethod]
    public void EmptyGridLayoutModelIsNotValid()
    {
        GridLayoutModel gridLayoutModel = new GridLayoutModel();
        Assert.IsFalse(gridLayoutModel.IsModelValid());
    }

    [TestMethod]
    public void GridLayoutModelWithInvalidRowAndColumnCountsIsNotValid()
    {
        GridLayoutModel gridLayoutModel = new GridLayoutModel();
        gridLayoutModel.Rows = 0;
        gridLayoutModel.Columns = 0;
        Assert.IsFalse(gridLayoutModel.IsModelValid());
    }

    [TestMethod]
    public void GridLayoutModelWithInvalidRowPercentsIsNotValid()
    {
        GridLayoutModel gridLayoutModel = new GridLayoutModel();
        gridLayoutModel.Rows = 1;

Review comment (suggestion):
```suggestion
namespace UnitTestsFancyZonesEditor;
```

The last missing change. They still have different namespaces.
 
![image](https://github.com/microsoft/PowerToys/assets/8949536/a76b3fdd-8858-438e-a809-8f149a6e6536)


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 FancyZonesEditor.Models;

namespace UnitTestsFancyZonesEditor;

[TestClass]
public class GridLayoutModelTests
{
    [TestMethod]
    public void EmptyGridLayoutModelIsNotValid()
    {
        GridLayoutModel gridLayoutModel = new GridLayoutModel();
        Assert.IsFalse(gridLayoutModel.IsModelValid());
    }

    [TestMethod]
    public void GridLayoutModelWithInvalidRowAndColumnCountsIsNotValid()
    {
        GridLayoutModel gridLayoutModel = new GridLayoutModel();
        gridLayoutModel.Rows = 0;
        gridLayoutModel.Columns = 0;
        Assert.IsFalse(gridLayoutModel.IsModelValid());
    }

    [TestMethod]
    public void GridLayoutModelWithInvalidRowPercentsIsNotValid()
    {
        GridLayoutModel gridLayoutModel = new GridLayoutModel();
        gridLayoutModel.Rows = 1;
Repository: microsoft/PowerToys
File: src/modules/colorPicker/ColorPickerUI/Mouse/IMouseInfoProvider.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.Drawing;

namespace ColorPicker.Mouse
{
    public interface IMouseInfoProvider
    {
        event EventHandler<Color> MouseColorChanged;

        event EventHandler<System.Windows.Point> MousePositionChanged;

        // position and bool indicating zoom in or zoom out
        event EventHandler<Tuple<System.Windows.Point, bool>> OnMouseWheel;

        event MouseUpEventHandler OnMouseDown;

        event RMouseUpEventHandler OnRMouseDown;

        System.Windows.Point CurrentPosition { get; }

        Color CurrentColor { get; }
    }
}


Review comment (suggestion):
while the hook is abbreviated and is RMouse, we should pivot the methods and properties in code to be maybe be more aligned with maybe either settings or something.  Secondary would be preferable as that is what is actually in settings. if someone switches buttons in settings, i'd hope this would just flip accordingly.

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.Drawing;

namespace ColorPicker.Mouse
{
    public interface IMouseInfoProvider
    {
        event EventHandler<Color> MouseColorChanged;

        event EventHandler<System.Windows.Point> MousePositionChanged;

        // position and bool indicating zoom in or zoom out
        event EventHandler<Tuple<System.Windows.Point, bool>> OnMouseWheel;

        event MouseUpEventHandler OnMouseDown;

        event SecondaryMouseUpEventHandler OnSecondaryMouseUp;

        System.Windows.Point CurrentPosition { get; }

        Color CurrentColor { get; }
    }
}
Repository: microsoft/PowerToys
File: src/modules/fancyzones/editor/FancyZonesEditor/LayoutBackup.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.Collections.Generic;
using FancyZonesEditor.Models;
using FancyZonesEditor.Utils;

namespace FancyZonesEditor
{
    public class LayoutBackup
    {
        private LayoutModel _backup;
        private string _hotkeyBackup;
        private Dictionary<int, LayoutModel> _defaultLayoutsBackup;

        public LayoutBackup()
        {
        }

        public void Backup(LayoutModel model)
        {
            if (model is GridLayoutModel grid)
            {
                _backup = new GridLayoutModel(grid);
            }
            else if (model is CanvasLayoutModel canvas)
            {
                _backup = new CanvasLayoutModel(canvas);
            }

            _hotkeyBackup = MainWindowSettingsModel.LayoutHotkeys.Key(model.Uuid);
            _defaultLayoutsBackup = new Dictionary<int, LayoutModel>(MainWindowSettingsModel.DefaultLayouts.Layouts);
        }

        public void Restore(LayoutModel layoutToRestore)
        {
            if (_backup != null && layoutToRestore != null)
            {
                if (_backup is GridLayoutModel grid)

Review comment (suggestion):
It'd be even better if the dictionary key would be a `MonitorConfigurationType` instead of `int`. It will be more readable and will eliminate casts to int.

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.Collections.Generic;
using FancyZonesEditor.Models;
using FancyZonesEditor.Utils;

namespace FancyZonesEditor
{
    public class LayoutBackup
    {
        private LayoutModel _backup;
        private string _hotkeyBackup;
        private Dictionary<MonitorConfigurationType, LayoutModel> _defaultLayoutsBackup;

        public LayoutBackup()
        {
        }

        public void Backup(LayoutModel model)
        {
            if (model is GridLayoutModel grid)
            {
                _backup = new GridLayoutModel(grid);
            }
            else if (model is CanvasLayoutModel canvas)
            {
                _backup = new CanvasLayoutModel(canvas);
            }

            _hotkeyBackup = MainWindowSettingsModel.LayoutHotkeys.Key(model.Uuid);
            _defaultLayoutsBackup = new Dictionary<MonitorConfigurationType, LayoutModel>(MainWindowSettingsModel.DefaultLayouts.Layouts);
        }

        public void Restore(LayoutModel layoutToRestore)
        {
            if (_backup != null && layoutToRestore != null)
            {
                if (_backup is GridLayoutModel grid)