ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/PowerToys
File: src/modules/peek/Peek.FilePreviewer/Previewers/PreviewerFactory.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 Peek.FilePreviewer.Previewers
{
    using Peek.Common.Models;

    public class PreviewerFactory
    {
        public IPreviewer? Create(File file)
        {
            if (ImagePreviewer.IsFileTypeSupported(file.Extension.ToLower()))
            {
                return new ImagePreviewer(file);
            }

            // Other previewer types check their supported file types here
            return null;
        }
    }
}


Review comment (suggestion):
1. Consider moving ToLower to the class `File` so we don't have to do it everywhere.

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 Peek.FilePreviewer.Previewers
{
    using Peek.Common.Models;

    public class PreviewerFactory
    {
        public IPreviewer? Create(File file)
        {
            if (ImagePreviewer.IsFileTypeSupported(file.Extension))
            {
                return new ImagePreviewer(file);
            }

            // Other previewer types check their supported file types here
            return null;
        }
    }
}
Repository: microsoft/PowerToys
File: src/modules/peek/Peek.UI/Extensions/WindowExtensions.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 Peek.UI.Extensions
{
    using System.Drawing;
    using Microsoft.UI.Xaml;
    using Windows.Win32;
    using Windows.Win32.Foundation;
    using Windows.Win32.Graphics.Gdi;
    using WinUIEx;

    public static class WindowExtensions
    {
        public static Size GetMonitorSize(this Window window)
        {
            var hwnd = new HWND(window.GetWindowHandle());
            var hwndDesktop = PInvoke.MonitorFromWindow(hwnd, MONITOR_FROM_FLAGS.MONITOR_DEFAULTTONEAREST);
            MONITORINFO info = new ();
            info.cbSize = 40;
            PInvoke.GetMonitorInfo(hwndDesktop, ref info);
            var monitorWidth = info.rcMonitor.left + info.rcMonitor.right;
            var monitorHeight = info.rcMonitor.bottom + info.rcMonitor.top;

            return new Size(monitorWidth, monitorHeight);
        }
    }
}


Review comment (suggestion):
Is it possible for the baseline dpi to not be 96 for some monitors? Seems like you can also use this api alternatively to avoid dealing with DPI at all: https://learn.microsoft.com/en-us/windows/win32/api/shellscalingapi/nf-shellscalingapi-getscalefactorformonitor

Code after:
    using Windows.Foundation;
    using Windows.Win32;
    using Windows.Win32.Foundation;
    using Windows.Win32.Graphics.Gdi;
    using WinUIEx;

    public static class WindowExtensions
    {
        public static Size GetMonitorSize(this Window window)
        {
            var hwnd = new HWND(window.GetWindowHandle());
            var hwndDesktop = PInvoke.MonitorFromWindow(hwnd, MONITOR_FROM_FLAGS.MONITOR_DEFAULTTONEAREST);
            MONITORINFO info = new ();
            info.cbSize = 40;
            PInvoke.GetMonitorInfo(hwndDesktop, ref info);
            double monitorWidth = info.rcMonitor.left + info.rcMonitor.right;
            double monitorHeight = info.rcMonitor.bottom + info.rcMonitor.top;

            return new Size(monitorWidth, monitorHeight);
        }

        public static double GetMonitorScale(this Window window)
        {
            var hwnd = new HWND(window.GetWindowHandle());
            var dpi = PInvoke.GetDpiForWindow(new HWND(hwnd));
            double scalingFactor = dpi / 96d;

            return scalingFactor;
        }
    }
}
Repository: microsoft/PowerToys
File: src/modules/peek/Peek.FilePreviewer/Previewers/ImagePreviewer/Helpers/ThumbnailHelper.cs (C#)

Code before:
            {
                var currentThumbnailFallBackIndex = ThumbnailFallBackSizes.IndexOf(thumbnailSize);
                var nextThumbnailFallBackIndex = currentThumbnailFallBackIndex + 1;
                if (nextThumbnailFallBackIndex < ThumbnailFallBackSizes.Count - 1)
                {
                    hr = GetThumbnail(filename, out hbitmap, ThumbnailFallBackSizes[nextThumbnailFallBackIndex]);
                }
            }

            Marshal.ReleaseComObject(nativeShellItem);

            return hr;
        }

        public static async Task<BitmapImage> GetThumbnailAsync(string path, uint size)
        {
            // preview image
            var file = await Windows.Storage.StorageFile.GetFileFromPathAsync(path);

            var imageStream = await file.GetThumbnailAsync(
                Windows.Storage.FileProperties.ThumbnailMode.SingleItem,
                size,
                Windows.Storage.FileProperties.ThumbnailOptions.None);

            BitmapImage bitmapImage = new BitmapImage();
            bitmapImage.SetSource(imageStream);

            return bitmapImage;
        }
    }
}


Review comment (performance):
How is the performance on this? If I recall correctly, the first iteration of the migration used the Windows Storage API and it was much slower than what we have currently

Code after:
            {
                var currentThumbnailFallBackIndex = ThumbnailFallBackSizes.IndexOf(thumbnailSize);
                var nextThumbnailFallBackIndex = currentThumbnailFallBackIndex + 1;
                if (nextThumbnailFallBackIndex < ThumbnailFallBackSizes.Count - 1)
                {
                    hr = GetThumbnail(filename, out hbitmap, ThumbnailFallBackSizes[nextThumbnailFallBackIndex]);
                }
            }

            Marshal.ReleaseComObject(nativeShellItem);

            return hr;
        }

        public static async Task<BitmapImage?> GetThumbnailAsync(string path, uint size)
        {
            BitmapImage? bitmapImage = null;

            // preview image
            var file = await Windows.Storage.StorageFile.GetFileFromPathAsync(path);
            if (file == null)
            {
                return bitmapImage;
            }

            var imageStream = await file.GetThumbnailAsync(
                Windows.Storage.FileProperties.ThumbnailMode.SingleItem,
                size,
                Windows.Storage.FileProperties.ThumbnailOptions.None);

            if (imageStream == null)
            {
                return bitmapImage;
            }

            bitmapImage = new BitmapImage();
            bitmapImage.SetSource(imageStream);

            return bitmapImage;
        }
    }
}
Repository: microsoft/PowerToys
File: .pipelines/verifyNoticeMdAgainstNugetPackages.ps1 (PowerShell)

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

$noticeFile = Get-Content -Raw "NOTICE.md"

Write-Host $noticeFile

Write-Host "Verifying NuGet packages"

$projFiles = Get-ChildItem $path -Include *.csproj -force -Recurse
$projFiles.Count
$totalList = New-Object System.Collections.ArrayList

Write-Host "Going through all csproj files"

foreach($csproj in $projFiles) 
{
    $nugetTemp = dotnet list $csproj package

    if($nugetTemp -is [array] -and $nugetTemp.count -gt 3)
    {
        $temp = New-Object System.Collections.ArrayList
        $temp.AddRange($nugetTemp)
        $temp.RemoveRange(0, 3)

        foreach($p in $temp) 
        {
            # breaking item down to usable array and getting 1 and 2, see below of a sample output
            #    > PACKAGE      VERSION            VERSION

            $p = -split $p
            $p = $p[1, 2]
            $tempString = $p[0] + " " + $p[1]

            if(![string]::IsNullOrWhiteSpace($tempString))

Review comment (suggestion):
```suggestion
$projFiles = Get-ChildItem $path -Filter *.csproj -force -Recurse
```
Using Filter instead of Include will speed this portion up significantly. 

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

$noticeFile = Get-Content -Raw "NOTICE.md"

Write-Host $noticeFile

Write-Host "Verifying NuGet packages"

$projFiles = Get-ChildItem $path -Filter *.csproj -force -Recurse
$projFiles.Count

Write-Host "Going through all csproj files"

$totalList = $projFiles | ForEach-Object -Parallel {
    $csproj = $_
    $nugetTemp = @();
    
    #Workaround for preventing exit code from dotnet process from reflecting exit code in PowerShell
    $procInfo = New-Object System.Diagnostics.ProcessStartInfo -Property @{ 
        FileName               = "dotnet.exe"; 
        Arguments              = "list $csproj package"; 
        RedirectStandardOutput = $true; 
        RedirectStandardError  = $true; 
    }
    
    $proc = [System.Diagnostics.Process]::Start($procInfo);

    while (!$proc.StandardOutput.EndOfStream) {
        $nugetTemp += $proc.StandardOutput.ReadLine();
    }
    
    $proc = $null;
    $procInfo = $null;
Repository: microsoft/PowerToys
File: .pipelines/verifyNoticeMdAgainstNugetPackages.ps1 (PowerShell)

Code before:
        foreach($p in $temp) 
        {
            # breaking item down to usable array and getting 1 and 2, see below of a sample output
            #    > PACKAGE      VERSION            VERSION

            $p = -split $p
            $p = $p[1, 2]
            $tempString = $p[0] + " " + $p[1]

            if(![string]::IsNullOrWhiteSpace($tempString))
            {
                $totalList.Add($tempString)
            }
        }
    }
}

Write-Host "Removing duplicates"

$totalList = $totalList | Sort-Object | Get-Unique
$returnList = ""

foreach($p in $totalList) 
{
    $returnList += "- " + $p + "`r`n"
}

Write-Host $returnList

if (!$noticeFile.Trim().EndsWith($returnList.Trim()))
{
	Write-Host -ForegroundColor Red "Notice.md does not match NuGet list."
	exit 1
}

exit 0


Review comment (suggestion):
Parallelize process

```suggestion
$totalList = $projFiles | ForEach-Object -Parallel {
    $csproj = $_
    $nugetTemp = dotnet list $csproj package

    if($nugetTemp -is [array] -and $nugetTemp.count -gt 3)
    {
        $temp = New-Object System.Collections.ArrayList
        $temp.AddRange($nugetTemp)
        $temp.RemoveRange(0, 3)

        foreach($p in $temp) 
        {
            # breaking item down to usable array and getting 1 and 2, see below of a sample output
            #    > PACKAGE      VERSION            VERSION

            $p = -split $p
            $p = $p[1, 2]
            $tempString = $p[0] + " " + $p[1]

            if(![string]::IsNullOrWhiteSpace($tempString))
            {
		   echo "- $tempString";
            }
        }
	$csproj = $null;
    }
} -ThrottleLimit 4 | Sort-Object

$returnList = [System.Collections.Generic.HashSet[string]]($totalList) -join "`r`n"
```

Code after:
    
    $proc = [System.Diagnostics.Process]::Start($procInfo);

    while (!$proc.StandardOutput.EndOfStream) {
        $nugetTemp += $proc.StandardOutput.ReadLine();
    }
    
    $proc = $null;
    $procInfo = $null;

    if($nugetTemp -is [array] -and $nugetTemp.count -gt 3)
    {
        $temp = New-Object System.Collections.ArrayList
        $temp.AddRange($nugetTemp)
        $temp.RemoveRange(0, 3)

        foreach($p in $temp) 
        {
            # breaking item down to usable array and getting 1 and 2, see below of a sample output
            #    > PACKAGE      VERSION            VERSION

            $p = -split $p
            $p = $p[1, 2]
            $tempString = $p[0] + " " + $p[1]

            if(![string]::IsNullOrWhiteSpace($tempString))
            {
		   echo "- $tempString";
            }
        }
	$csproj = $null;
    }
} -ThrottleLimit 4 | Sort-Object

$returnList = [System.Collections.Generic.HashSet[string]]($totalList) -join "`r`n"

Write-Host $returnList

if (!$noticeFile.Trim().EndsWith($returnList.Trim()))
{
	Write-Host -ForegroundColor Red "Notice.md does not match NuGet list."
	exit 1
}

exit 0
Repository: microsoft/PowerToys
File: src/settings-ui/Settings.UI/ViewModels/GeneralViewModel.cs (C#)

Code before:
        // Gets or sets a value indicating whether run powertoys on start-up.
        public bool Startup
        {
            get
            {
                return _startup;
            }

            set
            {
                if (_runAtStartupIsGPOConfigured)
                {
                    // If it's GPO configured, shouldn't be able to change this state.
                    return;
                }

                if (_startup != value)
                {
                    _startup = value;
                    GeneralSettingsConfig.Startup = value;
                    NotifyPropertyChanged();
                }
            }
        }

        // Gets or sets a value indicating whether the PowerToys icon should be hidden in the system tray.
        public bool ShowSysTrayIcon
        {
            get
            {
                return _showSysTrayIcon;
            }

            set
            {
                if (_showSysTrayIcon != value)
                {
                    _showSysTrayIcon = value;
                    GeneralSettingsConfig.ShowSysTrayIcon = value;
                    NotifyPropertyChanged();
                }
            }
        }

        public string RunningAsText
        {
            get
            {
                if (!IsElevated)
                {
                    return RunningAsUserDefaultText;

Review comment (suggestion):
```cs
// Gets or sets a value indicating whether the PowerToys icon should be shown in the system tray.
```

Code after:
        // Gets or sets a value indicating whether run powertoys on start-up.
        public bool Startup
        {
            get
            {
                return _startup;
            }

            set
            {
                if (_runAtStartupIsGPOConfigured)
                {
                    // If it's GPO configured, shouldn't be able to change this state.
                    return;
                }

                if (_startup != value)
                {
                    _startup = value;
                    GeneralSettingsConfig.Startup = value;
                    NotifyPropertyChanged();
                }
            }
        }

        // Gets or sets a value indicating whether the PowerToys icon should be shown in the system tray.
        public bool ShowSysTrayIcon
        {
            get
            {
                return _showSysTrayIcon;
            }

            set
            {
                if (_showSysTrayIcon != value)
                {
                    _showSysTrayIcon = value;
                    GeneralSettingsConfig.ShowSysTrayIcon = value;
                    NotifyPropertyChanged();
                }
            }
        }

        public string RunningAsText
        {
            get
            {
                if (!IsElevated)
                {
                    return RunningAsUserDefaultText;
Repository: microsoft/PowerToys
File: src/runner/tray_icon.cpp (C++)

Code before:
        tray_icon_data.uCallbackMessage = wm_icon_notify;

        std::wstringstream pt_version_tooltip_stream;
        if (isProcessElevated)
        {
            pt_version_tooltip_stream << GET_RESOURCE_STRING(IDS_TRAY_ICON_ADMIN_TOOLTIP) << L": ";
        }

        pt_version_tooltip_stream << L"PowerToys " << get_product_version() << '\0';
        std::wstring pt_version_tooltip = pt_version_tooltip_stream.str();
        wcscpy_s(tray_icon_data.szTip, sizeof(tray_icon_data.szTip) / sizeof(WCHAR), pt_version_tooltip.c_str());
        tray_icon_data.uFlags = NIF_ICON | NIF_TIP | NIF_MESSAGE;
        ChangeWindowMessageFilterEx(hwnd, WM_COMMAND, MSGFLT_ALLOW, nullptr);

        tray_icon_created = Shell_NotifyIcon(NIM_ADD, &tray_icon_data) == TRUE;
    }
}

void set_tray_icon_visible(bool shouldIconBeVisible)
{
    tray_icon_data.uFlags |= NIF_STATE;
    tray_icon_data.dwState = shouldIconBeVisible ? NIS_HIDDEN : 0;
    tray_icon_data.dwStateMask = NIS_HIDDEN;
    Shell_NotifyIcon(NIM_MODIFY, &tray_icon_data);
}

void stop_tray_icon()
{
    if (tray_icon_created)
    {
        SendMessage(tray_icon_hwnd, WM_CLOSE, 0, 0);
    }
}


Review comment (suggestion):
```cpp
void set_tray_icon_visible(bool shouldIconBeVisible)
{
    tray_icon_data.uFlags |= NIF_STATE; //  Declares that dwState and dwStateMask members are valid
    // dwState modifies the icon's actual state. Setting it to NIS_HIDDEN hides the icon.
    // dwStateMask specifies which bits of dwState are to be modified by the Shell_NotifyIcon call (should be the same as dwState typically.
    tray_icon_data.dwState = tray_icon_data.dwStateMask = shouldIconBeVisible ? 0 : NIS_HIDDEN;
    Shell_NotifyIcon(NIM_MODIFY, &tray_icon_data);
}
```

Code after:
        tray_icon_data.uCallbackMessage = wm_icon_notify;

        std::wstringstream pt_version_tooltip_stream;
        if (isProcessElevated)
        {
            pt_version_tooltip_stream << GET_RESOURCE_STRING(IDS_TRAY_ICON_ADMIN_TOOLTIP) << L": ";
        }

        pt_version_tooltip_stream << L"PowerToys " << get_product_version() << '\0';
        std::wstring pt_version_tooltip = pt_version_tooltip_stream.str();
        wcscpy_s(tray_icon_data.szTip, sizeof(tray_icon_data.szTip) / sizeof(WCHAR), pt_version_tooltip.c_str());
        tray_icon_data.uFlags = NIF_ICON | NIF_TIP | NIF_MESSAGE;
        ChangeWindowMessageFilterEx(hwnd, WM_COMMAND, MSGFLT_ALLOW, nullptr);

        tray_icon_created = Shell_NotifyIcon(NIM_ADD, &tray_icon_data) == TRUE;
    }
}

void set_tray_icon_visible(bool shouldIconBeVisible)
{
    tray_icon_data.uFlags |= NIF_STATE;
    tray_icon_data.dwStateMask = NIS_HIDDEN;
    tray_icon_data.dwState = shouldIconBeVisible ? 0 : NIS_HIDDEN;
    Shell_NotifyIcon(NIM_MODIFY, &tray_icon_data);
}

void stop_tray_icon()
{
    if (tray_icon_created)
    {
        SendMessage(tray_icon_hwnd, WM_CLOSE, 0, 0);
    }
}
Repository: microsoft/PowerToys
File: src/runner/general_settings.cpp (C++)

Code before:
#include "pch.h"
#include "general_settings.h"
#include "auto_start_helper.h"
#include "tray_icon.h"
#include "Generated files/resource.h"

#include <common/SettingsAPI/settings_helpers.h>
#include "powertoy_module.h"
#include <common/themes/windows_colors.h>

#include "trace.h"
#include <common/utils/elevation.h>
#include <common/version/version.h>
#include <common/utils/resources.h>

// TODO: would be nice to get rid of these globals, since they're basically cached json settings
static std::wstring settings_theme = L"system";
static bool show_tray_icon = false;
static bool run_as_elevated = false;
static bool show_new_updates_toast_notification = true;
static bool download_updates_automatically = true;
static bool show_whats_new_after_updates = true;
static bool enable_experimentation = true;
static bool enable_warnings_elevated_apps = true;

json::JsonObject GeneralSettings::to_json()
{
    json::JsonObject result;

    result.SetNamedValue(L"startup", json::value(isStartupEnabled));
    if (!startupDisabledReason.empty())
    {
        result.SetNamedValue(L"startup_disabled_reason", json::value(startupDisabledReason));
    }

    json::JsonObject enabled;
    for (const auto& [name, isEnabled] : isModulesEnabledMap)
    {
        enabled.SetNamedValue(name, json::value(isEnabled));
    }
    result.SetNamedValue(L"enabled", std::move(enabled));

    result.SetNamedValue(L"show_tray_icon", json::value(showSystemTrayIcon));

Review comment (suggestion):
```cpp
static bool show_tray_icon = true;
```

Default is to show the icon.

Code after:
#include "pch.h"
#include "general_settings.h"
#include "auto_start_helper.h"
#include "tray_icon.h"
#include "Generated files/resource.h"

#include <common/SettingsAPI/settings_helpers.h>
#include "powertoy_module.h"
#include <common/themes/windows_colors.h>

#include "trace.h"
#include <common/utils/elevation.h>
#include <common/version/version.h>
#include <common/utils/resources.h>

// TODO: would be nice to get rid of these globals, since they're basically cached json settings
static std::wstring settings_theme = L"system";
static bool show_tray_icon = true;
static bool run_as_elevated = false;
static bool show_new_updates_toast_notification = true;
static bool download_updates_automatically = true;
static bool show_whats_new_after_updates = true;
static bool enable_experimentation = true;
static bool enable_warnings_elevated_apps = true;

json::JsonObject GeneralSettings::to_json()
{
    json::JsonObject result;

    result.SetNamedValue(L"startup", json::value(isStartupEnabled));
    if (!startupDisabledReason.empty())
    {
        result.SetNamedValue(L"startup_disabled_reason", json::value(startupDisabledReason));
    }

    json::JsonObject enabled;
    for (const auto& [name, isEnabled] : isModulesEnabledMap)
    {
        enabled.SetNamedValue(name, json::value(isEnabled));
    }
    result.SetNamedValue(L"enabled", std::move(enabled));

    result.SetNamedValue(L"show_tray_icon", json::value(showSystemTrayIcon));
Repository: microsoft/PowerToys
File: src/modules/launcher/Plugins/Microsoft.Plugin.WindowWalker/Components/SearchController.cs (C#)

Code before:
                }
            }

            System.Diagnostics.Debug.Print("Found " + result.Count + " windows that match the search text");

            return result;
        }

        /// <summary>
        /// Search method that matches all the windows with a title
        /// </summary>
        /// <param name="openWindows">what windows are open</param>
        /// <returns>Returns search results</returns>
        private List<SearchResult> AllOpenWindows(List<Window> openWindows)
        {
            List<SearchResult> result = new List<SearchResult>();

            foreach (var window in openWindows)
            {
                if (window.Title.Length != 0)
                {
                    result.Add(new SearchResult(window));
                }
            }

            System.Diagnostics.Debug.Print("Found " + result.Count + " windows that match the search text");

            return result.OrderBy(w => w.Result.Title).ToList();
        }

        /// <summary>
        /// Event args for a window list update event
        /// </summary>
        internal class SearchResultUpdateEventArgs : EventArgs
        {
        }
    }
}


Review comment (suggestion):
Why not order by Z-Index. We can have a setting to switch between up or descending index order. (Can be a second step improvement.)

Code after:
                }
            }

            System.Diagnostics.Debug.Print("Found " + result.Count + " windows that match the search text");

            return result;
        }

        /// <summary>
        /// Search method that matches all the windows with a title
        /// </summary>
        /// <param name="openWindows">what windows are open</param>
        /// <returns>Returns search results</returns>
        private List<SearchResult> AllOpenWindows(List<Window> openWindows)
        {
            List<SearchResult> result = new List<SearchResult>();

            foreach (var window in openWindows)
            {
                if (window.Title.Length != 0)
                {
                    result.Add(new SearchResult(window));
                }
            }

            return result.OrderBy(w => w.Result.Title).ToList();
        }

        /// <summary>
        /// Event args for a window list update event
        /// </summary>
        internal class SearchResultUpdateEventArgs : EventArgs
        {
        }
    }
}