ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/PowerToys
File: src/modules/alwaysontop/AlwaysOnTop/AlwaysOnTop.h (C/C++)

Code before:
#pragma once

#include <map>

#include <Settings.h>
#include <SettingsObserver.h>
#include <Sound.h>
#include <VirtualDesktopUtils.h>
#include <WindowBorder.h>

#include <common/hooks/WinHookEvent.h>

class AlwaysOnTop : public SettingsObserver
{
public:
    AlwaysOnTop(bool powerToysPid);
    ~AlwaysOnTop();

protected:
    static LRESULT CALLBACK WndProc_Helper(HWND window, UINT message, WPARAM wparam, LPARAM lparam) noexcept
    {
        auto thisRef = reinterpret_cast<AlwaysOnTop*>(GetWindowLongPtr(window, GWLP_USERDATA));

        if (!thisRef && (message == WM_CREATE))
        {
            const auto createStruct = reinterpret_cast<LPCREATESTRUCT>(lparam);
            thisRef = reinterpret_cast<AlwaysOnTop*>(createStruct->lpCreateParams);
            SetWindowLongPtr(window, GWLP_USERDATA, reinterpret_cast<LONG_PTR>(thisRef));
        }

        return thisRef ? thisRef->WndProc(window, message, wparam, lparam) :
                         DefWindowProc(window, message, wparam, lparam);
    }

private:
    // IDs used to register hot keys (keyboard shortcuts).
    enum class HotkeyId : int
    {
        Pin = 1,
    };


Review comment (suggestion):
```suggestion
    AlwaysOnTop(bool useLLKH);
```

Code after:
#pragma once

#include <map>

#include <Settings.h>
#include <SettingsObserver.h>
#include <Sound.h>
#include <VirtualDesktopUtils.h>
#include <WindowBorder.h>

#include <common/hooks/WinHookEvent.h>

class AlwaysOnTop : public SettingsObserver
{
public:
    AlwaysOnTop(bool useLLKH);
    ~AlwaysOnTop();

protected:
    static LRESULT CALLBACK WndProc_Helper(HWND window, UINT message, WPARAM wparam, LPARAM lparam) noexcept
    {
        auto thisRef = reinterpret_cast<AlwaysOnTop*>(GetWindowLongPtr(window, GWLP_USERDATA));

        if (!thisRef && (message == WM_CREATE))
        {
            const auto createStruct = reinterpret_cast<LPCREATESTRUCT>(lparam);
            thisRef = reinterpret_cast<AlwaysOnTop*>(createStruct->lpCreateParams);
            SetWindowLongPtr(window, GWLP_USERDATA, reinterpret_cast<LONG_PTR>(thisRef));
        }

        return thisRef ? thisRef->WndProc(window, message, wparam, lparam) :
                         DefWindowProc(window, message, wparam, lparam);
    }

private:
    // IDs used to register hot keys (keyboard shortcuts).
    enum class HotkeyId : int
    {
        Pin = 1,
    };
Repository: microsoft/PowerToys
File: src/modules/launcher/Plugins/Microsoft.Plugin.Uri/Interfaces/IUriParser.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.

namespace Microsoft.Plugin.Uri.Interfaces
{
    public interface IUriParser
    {
        bool TryParse(string input, out System.Uri result, out bool isWebUri, out System.Uri secondResult);
    }
}


Review comment (suggestion):
```suggestion
        bool TryParse(string input, out System.Uri firstResult, out bool isFirstResultWebUri, out System.Uri secondResult);
```
To make it more clear.

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.

namespace Microsoft.Plugin.Uri.Interfaces
{
    public interface IUriParser
    {
        bool TryParse(string input, out System.Uri webUri, out System.Uri systemUri);
    }
}
Repository: microsoft/PowerToys
File: src/settings-ui/Settings.UI.Library/ViewModels/MeasureToolViewModel.cs (C#)

Code before:
            set
            {
                if (Settings.Properties.ActivationShortcut != value)
                {
                    Settings.Properties.ActivationShortcut = value;
                    NotifyPropertyChanged();
                }
            }
        }

        public void NotifyPropertyChanged([CallerMemberName] string propertyName = null)
        {
            OnPropertyChanged(propertyName);

            if (propertyName == nameof(ShowContinuousCaptureWarning))
            {
                // Don't trigger a settings update if the changed property is for visual notification.
                return;
            }

            SettingsUtils.SaveSettings(Settings.ToJsonString(), MeasureToolSettings.ModuleName);
        }

        public bool ShowContinuousCaptureWarning
        {
            get => IsEnabled & ContinuousCapture;
        }

        private Func<string, int> SendConfigMSG { get; }
    }
}


Review comment (suggestion):
that's a bitwise and, no?
```suggestion
            get => IsEnabled && ContinuousCapture;
```



Code after:
            set
            {
                if (Settings.Properties.ActivationShortcut != value)
                {
                    Settings.Properties.ActivationShortcut = value;
                    NotifyPropertyChanged();
                }
            }
        }

        public void NotifyPropertyChanged([CallerMemberName] string propertyName = null)
        {
            OnPropertyChanged(propertyName);

            if (propertyName == nameof(ShowContinuousCaptureWarning))
            {
                // Don't trigger a settings update if the changed property is for visual notification.
                return;
            }

            SettingsUtils.SaveSettings(Settings.ToJsonString(), MeasureToolSettings.ModuleName);
        }

        public bool ShowContinuousCaptureWarning
        {
            get => IsEnabled && ContinuousCapture;
        }

        private Func<string, int> SendConfigMSG { get; }
    }
}
Repository: microsoft/PowerToys
File: src/modules/poweraccent/PowerAccent.Core/PowerAccent.cs (C#)

Code before:
                _selectedIndex = 0;
            }

            if (_selectedIndex > _characters.Length - 1)
            {
                _selectedIndex = _characters.Length - 1;
            }

            OnSelectCharacter?.Invoke(_selectedIndex, _characters[_selectedIndex]);
            return;
        }

        if (triggerKey == TriggerKey.Space)
        {
            if (WindowsFunctions.IsShiftState())
            {
                if (_selectedIndex < 0)
                {
                    _selectedIndex = _characters.Length - 1;
                }
                else
                {
                    --_selectedIndex;
                }
            }
            else
            {
                if (_selectedIndex < _characters.Length - 1)
                {
                    ++_selectedIndex;
                }
                else
                {
                    _selectedIndex = 0;
                }
            }
        }

        if (triggerKey == TriggerKey.Left)
        {
            --_selectedIndex;
        }

        if (triggerKey == TriggerKey.Right)
        {
            ++_selectedIndex;
        }

        // Wrap around at beginning and end of _selectedIndex range
        if (_selectedIndex < 0)
        {

Review comment (suggestion):
We'd prefer not to include this behavior, since pressing shift on the context of inserting a character means capitalizing it. Please remove it.

Code after:
                _selectedIndex = 0;
            }

            if (_selectedIndex > _characters.Length - 1)
            {
                _selectedIndex = _characters.Length - 1;
            }

            OnSelectCharacter?.Invoke(_selectedIndex, _characters[_selectedIndex]);
            return;
        }

        if (triggerKey == TriggerKey.Space)
        {
            if (_selectedIndex < _characters.Length - 1)
            {
                ++_selectedIndex;
            }
            else
            {
                _selectedIndex = 0;
            }
        }

        if (triggerKey == TriggerKey.Left)
        {
            --_selectedIndex;
        }

        if (triggerKey == TriggerKey.Right)
        {
            ++_selectedIndex;
        }

        // Wrap around at beginning and end of _selectedIndex range
        if (_selectedIndex < 0)
        {
            _selectedIndex = _characters.Length - 1;
        }

        if (_selectedIndex > _characters.Length - 1)
        {
            _selectedIndex = 0;
        }

        OnSelectCharacter?.Invoke(_selectedIndex, _characters[_selectedIndex]);
    }

    public Point GetDisplayCoordinates(Size window)
    {
        (Point Location, Size Size, double Dpi) activeDisplay = WindowsFunctions.GetActiveDisplay();
Repository: microsoft/PowerToys
File: src/modules/alwaysontop/AlwaysOnTop/AlwaysOnTop.cpp (C++)

Code before:
    {
        auto iter = m_topmostWindows.find(data->hwnd);
        if (iter != m_topmostWindows.end())
        {
            const auto& border = iter->second;
            if (border)
            {
                border->UpdateBorderPosition();
            }
        }
    }
    break;
    case EVENT_SYSTEM_FOREGROUND:
    {
        RefreshBorders();
    }
    break;
    case EVENT_OBJECT_FOCUS:
    {
        for (const auto& [window, border] : m_topmostWindows)
        {
            // check if topmost was reset
            // fixes https://github.com/microsoft/PowerToys/issues/19168
            if (!IsTopmost(window))
            {
                Logger::trace(L"Reset topmost");
                PinTopmostWindow(window);
            }
        }
    }
    break;
    default:
        break;
    }
}

void AlwaysOnTop::RefreshBorders()
{
    for (const auto& [window, border] : m_topmostWindows)
    {
        if (m_virtualDesktopUtils.IsWindowOnCurrentDesktop(window))
        {
            if (!border)
            {
                AssignBorder(window);
            }
        }
        else
        {
            if (border)
            {

Review comment (suggestion):
Perhaps a more descriptive message? Like "A window no longer has Topmost set and it should. Setting topmost again."

Code after:
    {
        auto iter = m_topmostWindows.find(data->hwnd);
        if (iter != m_topmostWindows.end())
        {
            const auto& border = iter->second;
            if (border)
            {
                border->UpdateBorderPosition();
            }
        }
    }
    break;
    case EVENT_SYSTEM_FOREGROUND:
    {
        RefreshBorders();
    }
    break;
    case EVENT_OBJECT_FOCUS:
    {
        for (const auto& [window, border] : m_topmostWindows)
        {
            // check if topmost was reset
            // fixes https://github.com/microsoft/PowerToys/issues/19168
            if (!IsTopmost(window))
            {
                Logger::trace(L"A window no longer has Topmost set and it should. Setting topmost again.");
                PinTopmostWindow(window);
            }
        }
    }
    break;
    default:
        break;
    }
}

void AlwaysOnTop::RefreshBorders()
{
    for (const auto& [window, border] : m_topmostWindows)
    {
        if (m_virtualDesktopUtils.IsWindowOnCurrentDesktop(window))
        {
            if (!border)
            {
                AssignBorder(window);
            }
        }
        else
        {
            if (border)
            {
Repository: microsoft/PowerToys
File: tools/BugReportTool/BugReportTool/InstallationFolder.cpp (C++)

Code before:

	if (INVALID_HANDLE_VALUE == hFile)
	{
		return L"CreateFile() failed. " + get_last_error_or_default(GetLastError());
	}

	// Get handle to the crypto provider
	if (!CryptAcquireContext(&hProv,
		NULL,
		NULL,
		PROV_RSA_FULL,
		CRYPT_VERIFYCONTEXT))
	{
		CloseHandle(hFile);
		return L"CryptAcquireContext() failed. " + get_last_error_or_default(GetLastError());
	}

	if (!CryptCreateHash(hProv, CALG_MD5, 0, 0, &hHash))
	{
		CloseHandle(hFile);
		CryptReleaseContext(hProv, 0);
		return L"CryptCreateHash() failed. " + get_last_error_or_default(GetLastError());
	}

	bResult = ReadFile(hFile, rgbFile, bufferSize, &cbRead, NULL);
	while (bResult)
	{
		if (0 == cbRead)
		{
			break;
		}

		if (!CryptHashData(hHash, rgbFile, cbRead, 0))
		{
			CryptReleaseContext(hProv, 0);
			CryptDestroyHash(hHash);
			CloseHandle(hFile);
			return L"CryptHashData() failed. " + get_last_error_or_default(GetLastError());;
		}
	}

	if (!bResult)
	{
		CryptReleaseContext(hProv, 0);
		CryptDestroyHash(hHash);
		CloseHandle(hFile);
		return L"ReadFile() failed. " + get_last_error_or_default(GetLastError());;
	}

	cbHash = md5Length;
	std::wstring result = L"";

Review comment (bug):
This change looks wrong. It looks like an infinite cycle like this since bResult wouldn't be recalculated.

Code after:

	if (INVALID_HANDLE_VALUE == hFile)
	{
		return L"CreateFile() failed. " + get_last_error_or_default(GetLastError());
	}

	// Get handle to the crypto provider
	if (!CryptAcquireContext(&hProv,
		NULL,
		NULL,
		PROV_RSA_FULL,
		CRYPT_VERIFYCONTEXT))
	{
		CloseHandle(hFile);
		return L"CryptAcquireContext() failed. " + get_last_error_or_default(GetLastError());
	}

	if (!CryptCreateHash(hProv, CALG_MD5, 0, 0, &hHash))
	{
		CloseHandle(hFile);
		CryptReleaseContext(hProv, 0);
		return L"CryptCreateHash() failed. " + get_last_error_or_default(GetLastError());
	}

	bResult = ReadFile(hFile, rgbFile, bufferSize, &cbRead, NULL);
	while (bResult)
	{
		if (0 == cbRead)
		{
			break;
		}

		if (!CryptHashData(hHash, rgbFile, cbRead, 0))
		{
			CryptReleaseContext(hProv, 0);
			CryptDestroyHash(hHash);
			CloseHandle(hFile);
			return L"CryptHashData() failed. " + get_last_error_or_default(GetLastError());;
		}

		bResult = ReadFile(hFile, rgbFile, bufferSize, &cbRead, NULL);
	}

	if (!bResult)
	{
		CryptReleaseContext(hProv, 0);
		CryptDestroyHash(hHash);
		CloseHandle(hFile);
		return L"ReadFile() failed. " + get_last_error_or_default(GetLastError());;
	}
Repository: microsoft/PowerToys
File: .pipelines/verifyNugetPackages.ps1 (PowerShell)

Code before:
[CmdletBinding()]
Param(
    [Parameter(Mandatory=$True,Position=1)]
    [string]$solution
)

Write-Host "Verifying Nuget packages for $solution"

dotnet tool restore
dotnet consolidate -s $solution

if (-not $?)
{
    Write-Host -ForegroundColor Red "Nuget packages needs to be consolidated"
    exit 1
}

exit 0


Review comment (suggestion):
```suggestion
    Write-Host -ForegroundColor Red "Nuget packages with the same name must all be the same version."
```

Code after:
[CmdletBinding()]
Param(
    [Parameter(Mandatory=$True,Position=1)]
    [string]$solution
)

Write-Host "Verifying Nuget packages for $solution"

dotnet tool restore
dotnet consolidate -s $solution

if (-not $?)
{
    Write-Host -ForegroundColor Red "Nuget packages with the same name must all be the same version."
    exit 1
}

exit 0
Repository: microsoft/PowerToys
File: src/modules/fancyzones/FancyZonesTests/UnitTests/AppZoneHistoryTests.Spec.cpp (C++)

Code before:
#include "pch.h"
#include <filesystem>

// disabling warning 4702 - unreachable code
// prevent the warning on functions not used in tests.
#pragma warning(push)
#pragma warning(disable : 4702)
#include <FancyZonesLib/FancyZonesData/AppZoneHistory.h>
#pragma warning(pop)


#include "util.h"
#include <modules/fancyzones/FancyZonesLib/util.h>

using namespace Microsoft::VisualStudio::CppUnitTestFramework;

namespace FancyZonesUnitTests
{
    TEST_CLASS (AppZoneHistoryUnitTests)
    {
        HINSTANCE m_hInst{};

        TEST_METHOD_INITIALIZE(Init)
        {
            m_hInst = (HINSTANCE)GetModuleHandleW(nullptr);
            AppZoneHistory::instance().LoadData();
        }

        TEST_METHOD_CLEANUP(CleanUp)
        {
            std::filesystem::remove(AppZoneHistory::instance().AppZoneHistoryFileName());
        }

        TEST_METHOD (AppZoneHistoryParse)
        {

Review comment (suggestion):
I think it would be better to update AppZoneHistory.h.
If you add `#else` in `AppZoneHistoryFileName` disabling wouldn't be necessary.

```
#if defined(UNIT_TESTS)
        return ...;
#else
        return ...;
#endif
```

The same for others, such as AppliedLayout, CustomLayouts, LayoutHotkeys, etc. 

Code after:
#include "pch.h"
#include <filesystem>

#include <FancyZonesLib/FancyZonesData/AppZoneHistory.h>

#include "util.h"
#include <modules/fancyzones/FancyZonesLib/util.h>

using namespace Microsoft::VisualStudio::CppUnitTestFramework;

namespace FancyZonesUnitTests
{
    TEST_CLASS (AppZoneHistoryUnitTests)
    {
        HINSTANCE m_hInst{};

        TEST_METHOD_INITIALIZE(Init)
        {
            m_hInst = (HINSTANCE)GetModuleHandleW(nullptr);
            AppZoneHistory::instance().LoadData();
        }

        TEST_METHOD_CLEANUP(CleanUp)
        {
            std::filesystem::remove(AppZoneHistory::instance().AppZoneHistoryFileName());
        }

        TEST_METHOD (AppZoneHistoryParse)
        {
            // prepare
            json::JsonObject root{};
            json::JsonArray appZoneHistoryArray{};

            {
                json::JsonArray history{};
Repository: microsoft/PowerToys
File: src/modules/poweraccent/PowerAccent.UI/App.xaml.cs (C#)

Code before:

            if (!createdNew)
            {
                Logger.LogWarning("Another running QuickAccent instance was detected. Exiting QuickAccent");
                Application.Current.Shutdown();
            }

            _themeManager = new ThemeManager(this);
            base.OnStartup(e);
        }

        protected override void OnExit(ExitEventArgs e)
        {
            _mutex?.ReleaseMutex();
            base.OnExit(e);
        }

        protected virtual void Dispose(bool disposing)
        {
            if (!disposedValue)
            {
                _mutex?.Dispose();
                _themeManager?.Dispose();

                // TODO: free unmanaged resources (unmanaged objects) and override finalizer
                // TODO: set large fields to null
                disposedValue = true;
            }
        }

        public void Dispose()
        {
            // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method
            Dispose(disposing: true);
            GC.SuppressFinalize(this);
        }
    }
}


Review comment (suggestion):
I think that these comments are not relevant anymore right?
In addition, you may consider early return if disposedValue is true.

Code after:

            if (!createdNew)
            {
                Logger.LogWarning("Another running QuickAccent instance was detected. Exiting QuickAccent");
                Application.Current.Shutdown();
            }

            _themeManager = new ThemeManager(this);
            base.OnStartup(e);
        }

        protected override void OnExit(ExitEventArgs e)
        {
            _mutex?.ReleaseMutex();
            base.OnExit(e);
        }

        protected virtual void Dispose(bool disposing)
        {
            if (_disposed)
            {
                return;
            }

            if (disposing)
            {
                _mutex?.Dispose();
                _themeManager?.Dispose();
            }

            _disposed = true;
        }

        public void Dispose()
        {
            Dispose(disposing: true);
            GC.SuppressFinalize(this);
        }
    }
}
Repository: microsoft/PowerToys
File: src/modules/poweraccent/PowerAccent.UI/App.xaml.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.Threading;
using System.Windows;
using Common.UI;
using PowerAccent.Core.Tools;

namespace PowerAccent.UI
{
    /// <summary>
    /// Interaction logic for App.xaml
    /// </summary>
    public partial class App : Application, IDisposable
    {
        private static Mutex _mutex;
        private bool disposedValue;
        private ThemeManager _themeManager;

        protected override void OnStartup(StartupEventArgs e)
        {
            _mutex = new Mutex(true, "QuickAccent", out bool createdNew);

            if (!createdNew)
            {
                Logger.LogWarning("Another running QuickAccent instance was detected. Exiting QuickAccent");
                Application.Current.Shutdown();
            }

            _themeManager = new ThemeManager(this);
            base.OnStartup(e);
        }

        protected override void OnExit(ExitEventArgs e)
        {
            _mutex?.ReleaseMutex();
            base.OnExit(e);
        }

        protected virtual void Dispose(bool disposing)
        {
            if (!disposedValue)

Review comment (suggestion):
```suggestion
        private bool _disposed;
```

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.Threading;
using System.Windows;
using Common.UI;
using PowerAccent.Core.Tools;

namespace PowerAccent.UI
{
    /// <summary>
    /// Interaction logic for App.xaml
    /// </summary>
    public partial class App : Application, IDisposable
    {
        private static Mutex _mutex;
        private bool _disposed;
        private ThemeManager _themeManager;

        protected override void OnStartup(StartupEventArgs e)
        {
            _mutex = new Mutex(true, "QuickAccent", out bool createdNew);

            if (!createdNew)
            {
                Logger.LogWarning("Another running QuickAccent instance was detected. Exiting QuickAccent");
                Application.Current.Shutdown();
            }

            _themeManager = new ThemeManager(this);
            base.OnStartup(e);
        }

        protected override void OnExit(ExitEventArgs e)
        {
            _mutex?.ReleaseMutex();
            base.OnExit(e);
        }

        protected virtual void Dispose(bool disposing)
        {
            if (_disposed)
Repository: microsoft/PowerToys
File: src/modules/peek/Peek.FilePreviewer/FilePreview.xaml.cs (C#)

Code before:
            set => SetValue(FilesProperty, value);
        }

        private async Task OnFilePropertyChanged()
        {
            if (File == null)
            {
                return;
            }

            Previewer = previewerFactory.Create(File);
            if (Previewer != null)
            {
                var size = await Previewer.GetPreviewSizeAsync();
                PreviewSizeChanged?.Invoke(this, new PreviewSizeChangedArgs(size));
                await Previewer.LoadPreviewAsync();
            }
            else
            {
                // TODO: figure out optimal window size for unsupported control
                PreviewSizeChanged?.Invoke(this, new PreviewSizeChangedArgs(new Size(1280, 720)));
            }
        }

        private void PreviewBrowser_NavigationCompleted(WebView2 sender, Microsoft.Web.WebView2.Core.CoreWebView2NavigationCompletedEventArgs args)
        {
            // Once browser has completed navigation it is ready to be visible
            OnPropertyChanged(nameof(IsBrowserVisible));
        }
    }
}


Review comment (suggestion):
FYI: In my PR I rework how we trigger UI changes for control visibility so I think we won't need this event handler. 

Code after:
            }
        }

        public File File
        {
            get => (File)GetValue(FilesProperty);
            set => SetValue(FilesProperty, value);
        }

        private async Task OnFilePropertyChanged()
        {
            if (File == null)
            {
                return;
            }

            Previewer = previewerFactory.Create(File);
            if (Previewer != null)
            {
                var size = await Previewer.GetPreviewSizeAsync();
                PreviewSizeChanged?.Invoke(this, new PreviewSizeChangedArgs(size));
                await Previewer.LoadPreviewAsync();
            }
        }

        private void PreviewBrowser_NavigationCompleted(WebView2 sender, Microsoft.Web.WebView2.Core.CoreWebView2NavigationCompletedEventArgs args)
        {
            // Once browser has completed navigation it is ready to be visible
            OnPropertyChanged(nameof(IsBrowserVisible));
        }
    }
}