ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/PowerToys
File: src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelViewModel.cs (C#)

Code before:
    private void Item_PropertyChanged(object? sender, System.ComponentModel.PropertyChangedEventArgs e)
    {
        if (!string.IsNullOrEmpty(e.PropertyName))
        {
            PropChanged?.Invoke(this, new PropChangedEventArgs(e.PropertyName));

            if (e.PropertyName == "IsInitialized")
            {
                GenerateId();

                UpdateAlias();
                UpdateHotkey();
                UpdateTags();
            }
        }
    }

    private void Save() => SettingsModel.SaveSettings(_settings);

    private void HandleChangeAlias()
    {
        SetAlias(Alias);
        Save();
    }

    public void SetAlias(CommandAlias? newAlias)
    {
        _serviceProvider.GetService<AliasManager>()!.UpdateAlias(Id, newAlias);
        UpdateAlias();
        UpdateTags();
    }

    private void UpdateAlias()
    {
        // Add tags for the alias, if we have one.
        var aliases = _serviceProvider.GetService<AliasManager>();
        if (aliases != null)
        {
            Alias = aliases.AliasFromId(Id);
        }
    }

    private void UpdateHotkey()
    {
        var hotkey = _settings.CommandHotkeys.Where(hk => hk.CommandId == Id).FirstOrDefault();
        if (hotkey != null)
        {
            _hotkey = hotkey.Hotkey;
        }
    }


Review comment (suggestion):
@vanzue , it doesn't make sense this is a "Public" method, and become like using as "private method" (e.g. assuming calling from HandleChangeAlias() (which is a private method)

Code after:
    private void Item_PropertyChanged(object? sender, System.ComponentModel.PropertyChangedEventArgs e)
    {
        if (!string.IsNullOrEmpty(e.PropertyName))
        {
            PropChanged?.Invoke(this, new PropChangedEventArgs(e.PropertyName));

            if (e.PropertyName == "IsInitialized")
            {
                GenerateId();

                FetchAliasFromAliasManager();
                UpdateHotkey();
                UpdateTags();
            }
        }
    }

    private void Save() => SettingsModel.SaveSettings(_settings);

    private void HandleChangeAlias()
    {
        SetAlias();
        Save();
    }

    public void SetAlias()
    {
        var commandAlias = Alias is null
                ? null
                : new CommandAlias(Alias.Alias, Alias.CommandId, Alias.IsDirect);

        _serviceProvider.GetService<AliasManager>()!.UpdateAlias(Id, commandAlias);
        UpdateTags();
    }

    private void FetchAliasFromAliasManager()
    {
        var am = _serviceProvider.GetService<AliasManager>();
        if (am != null)
        {
            var commandAlias = am.AliasFromId(Id);
            if (commandAlias is not null)
            {
                // Decouple from the alias manager alias object
                Alias = new CommandAlias(commandAlias.Alias, commandAlias.CommandId, commandAlias.IsDirect);
            }
        }
    }

    private void UpdateHotkey()
    {
Repository: microsoft/PowerToys
File: src/modules/cmdpal/Exts/Microsoft.CmdPal.Ext.WebSearch/WebSearchCommandsProvider.cs (C#)

Code before:
    private readonly FallbackExecuteSearchItem _fallbackItem;
    private readonly FallbackOpenURLItem _openUrlFallbackItem;

    public WebSearchCommandsProvider()
    {
        Id = "WebSearch";
        DisplayName = Resources.extension_name;
        Icon = IconHelpers.FromRelativePath("Assets\\WebSearch.png");
        Settings = _settingsManager.Settings;

        _fallbackItem = new FallbackExecuteSearchItem(_settingsManager);
        _openUrlFallbackItem = new FallbackOpenURLItem(_settingsManager);
    }

    public override ICommandItem[] TopLevelCommands()
    {
        return [new WebSearchTopLevelCommandItem(_settingsManager)
        {
            MoreCommands = [
                new CommandContextItem(Settings!.SettingsPage),
            ],
        }
        ];
    }

    public override IFallbackCommandItem[]? FallbackCommands() => [_fallbackItem, _openUrlFallbackItem];
}


Review comment (suggestion):
I'd put the OpenUrlFallbackItem first, so that `google.com` will get you `https://google.com` first, **then** "search the web for google.com"

Code after:
    private readonly FallbackExecuteSearchItem _fallbackItem;
    private readonly FallbackOpenURLItem _openUrlFallbackItem;

    public WebSearchCommandsProvider()
    {
        Id = "WebSearch";
        DisplayName = Resources.extension_name;
        Icon = IconHelpers.FromRelativePath("Assets\\WebSearch.png");
        Settings = _settingsManager.Settings;

        _fallbackItem = new FallbackExecuteSearchItem(_settingsManager);
        _openUrlFallbackItem = new FallbackOpenURLItem(_settingsManager);
    }

    public override ICommandItem[] TopLevelCommands()
    {
        return [new WebSearchTopLevelCommandItem(_settingsManager)
        {
            MoreCommands = [
                new CommandContextItem(Settings!.SettingsPage),
            ],
        }
        ];
    }

    public override IFallbackCommandItem[]? FallbackCommands() => [_openUrlFallbackItem, _fallbackItem];
}
Repository: microsoft/PowerToys
File: src/modules/cmdpal/CmdPalModuleInterface/dllmain.cpp (C++)

Code before:
        if (processIds.size() == 0)
        {
            Logger::trace(L"Nothing To PROCESS_TERMINATE");
            return;
        }

        for (DWORD pid : processIds)
        {
            HANDLE hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, pid);

            if (hProcess != NULL)
            {
                SetEvent(m_hTerminateEvent);

                // Wait for 1.5 seconds for the process to end correctly and stop etw tracer
                WaitForSingleObject(hProcess, 1500);

                TerminateProcess(hProcess, 0);
                CloseHandle(hProcess);
            }
        }
    }

public:
    static std::atomic<bool> m_enabled;
    static std::atomic<bool> m_launched;

    CmdPal()
    {
        app_name = L"CmdPal";
        app_key = L"CmdPal";
        LoggerHelpers::init_logger(app_key, L"ModuleInterface", "CmdPal");

        m_hTerminateEvent = CreateDefaultEvent(CommonSharedConstants::CMDPAL_EXIT_EVENT);
    }

    ~CmdPal()
    {
        if (CmdPal::m_enabled)
        {
        }
        CmdPal::m_enabled.store(false);
    }

    // Destroy the powertoy and free memory
    virtual void destroy() override
    {
        Logger::trace("CmdPal::destroy()");
        TerminateCmdPal();
        delete this;
    }

Review comment (suggestion):
Given, it is just a bool, we don't have problem on concurrent read here.
For concurrent write, we also don't have, thus I think we don't need to make it as atomic here.

instead, they can be just like the firstEnableCall

Code after:

        if (processIds.size() == 0)
        {
            Logger::trace(L"Nothing To PROCESS_TERMINATE");
            return;
        }

        for (DWORD pid : processIds)
        {
            HANDLE hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, pid);

            if (hProcess != NULL)
            {
                SetEvent(m_hTerminateEvent);

                // Wait for 1.5 seconds for the process to end correctly and stop etw tracer
                WaitForSingleObject(hProcess, 1500);

                TerminateProcess(hProcess, 0);
                CloseHandle(hProcess);
            }
        }
    }

public:
    static std::atomic<bool> m_enabled;
    static std::atomic<bool> m_launched;

    CmdPal()
    {
        app_name = L"CmdPal";
        app_key = L"CmdPal";
        LoggerHelpers::init_logger(app_key, L"ModuleInterface", "CmdPal");

        m_hTerminateEvent = CreateDefaultEvent(CommonSharedConstants::CMDPAL_EXIT_EVENT);
    }

    ~CmdPal()
    {
        CmdPal::m_enabled.store(false);
    }

    // Destroy the powertoy and free memory
    virtual void destroy() override
    {
        Logger::trace("CmdPal::destroy()");
        TerminateCmdPal();
        delete this;
    }

    // Return the localized display name of the powertoy
Repository: microsoft/PowerToys
File: src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WindowsTerminal/Helpers/IApplicationActivationManager.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.Runtime.InteropServices;
using System.Runtime.InteropServices.Marshalling;

namespace Microsoft.CmdPal.Ext.WindowsTerminal.Helpers;

// Reference : https://github.com/MicrosoftEdge/edge-launcher/blob/108e63df0b4cb5cd9d5e45aa7a264690851ec51d/MIcrosoftEdgeLauncherCsharp/Program.cs
[Flags]
public enum ActivateOptions
{
    None = 0x00000000,
    DesignMode = 0x00000001,
    NoErrorUI = 0x00000002,
    NoSplashScreen = 0x00000004,
}

// ApplicationActivationManager
[GeneratedComInterface(StringMarshalling = StringMarshalling.Utf16)]
[Guid("2e941141-7f97-4756-ba1d-9decde894a3d")]
[InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
public partial interface IApplicationActivationManager
{
    IntPtr ActivateApplication(string appUserModelId, string arguments, ActivateOptions options, out uint processId);

    IntPtr ActivateForFile(string appUserModelId, IntPtr /*IShellItemArray* */ itemArray, string verb, out uint processId);

    IntPtr ActivateForProtocol(string appUserModelId, IntPtr /* IShellItemArray* */itemArray, out uint processId);
}


Review comment (suggestion):
we could remove the InterfaceType here, It's [IUKnown only ](https://learn.microsoft.com/en-us/dotnet/standard/native-interop/comwrappers-source-generation#iunknown-only)

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.Runtime.InteropServices;
using System.Runtime.InteropServices.Marshalling;

namespace Microsoft.CmdPal.Ext.WindowsTerminal.Helpers;

// Reference : https://github.com/MicrosoftEdge/edge-launcher/blob/108e63df0b4cb5cd9d5e45aa7a264690851ec51d/MIcrosoftEdgeLauncherCsharp/Program.cs
[Flags]
public enum ActivateOptions
{
    None = 0x00000000,
    DesignMode = 0x00000001,
    NoErrorUI = 0x00000002,
    NoSplashScreen = 0x00000004,
}

// ApplicationActivationManager
[GeneratedComInterface(StringMarshalling = StringMarshalling.Utf16)]
[Guid("2e941141-7f97-4756-ba1d-9decde894a3d")]
public partial interface IApplicationActivationManager
{
    void ActivateApplication(string appUserModelId, string arguments, ActivateOptions options, out uint processId);

    void ActivateForFile(string appUserModelId, IntPtr /*IShellItemArray* */ itemArray, string verb, out uint processId);

    void ActivateForProtocol(string appUserModelId, IntPtr /* IShellItemArray* */itemArray, out uint processId);
}
Repository: microsoft/PowerToys
File: src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ContentIcon.cs (C#)

Code before:
        get => (UIElement)GetValue(ContentProperty);
        set => SetValue(ContentProperty, value);
    }

    public static readonly DependencyProperty ContentProperty =
        DependencyProperty.Register(
            nameof(Content),
            typeof(UIElement),
            typeof(ContentIcon),
            new PropertyMetadata(null));

    public ContentIcon()
    {
        Loaded += IconBoxElement_Loaded;
    }

    private void IconBoxElement_Loaded(object sender, RoutedEventArgs e)
    {
        if (this.FindDescendants().OfType<Grid>().FirstOrDefault() is Grid grid && Content is not null)
        {
            if (grid.Children.Contains(Content))
            {
                return;
            }

            if (Content is FrameworkElement { Parent: not null } element)
            {
                Debug.Assert(false, $"IconBoxElement Content is already parented to {element.Parent.GetType().Name}");
                return;
            }

            grid.Children.Add(Content);
        }
    }
}


Review comment (suggestion):
_what the fuck did I just read_


like, I get what it's trying to say, but _man_ that's some wacky syntax C#. Is that basically the same as
```suggestion
            if (Content is FrameworkElement element && element.Parent is not null)
```

Code after:
        get => (UIElement)GetValue(ContentProperty);
        set => SetValue(ContentProperty, value);
    }

    public static readonly DependencyProperty ContentProperty =
        DependencyProperty.Register(
            nameof(Content),
            typeof(UIElement),
            typeof(ContentIcon),
            new PropertyMetadata(null));

    public ContentIcon()
    {
        Loaded += IconBoxElement_Loaded;
    }

    private void IconBoxElement_Loaded(object sender, RoutedEventArgs e)
    {
        if (this.FindDescendants().OfType<Grid>().FirstOrDefault() is Grid grid && Content is not null)
        {
            if (grid.Children.Contains(Content))
            {
                return;
            }

            if (Content is FrameworkElement element && element.Parent is not null)
            {
                Debug.Assert(false, $"IconBoxElement Content is already parented to {element.Parent.GetType().Name}");
                return;
            }

            grid.Children.Add(Content);
        }
    }
}
Repository: microsoft/PowerToys
File: src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Apps/Icons.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 Microsoft.CommandPalette.Extensions.Toolkit;

namespace Microsoft.CmdPal.Ext.Apps;

internal sealed class Icons
{
    internal static IconInfo AllAppsIcon => IconHelpers.FromRelativePath("Assets\\AllApps.svg");

    internal static IconInfo RunAsUserIcon => new("\uE7EE"); // OtherUser icon

    internal static IconInfo RunAsAdminIcon => new("\uE7EF"); // Admin icon

    internal static IconInfo OpenPathIcon => new("\ue838"); // Folder Open icon

    internal static IconInfo CopyIcon => new("\ue8c8"); // Copy icon

    public static IconInfo UnpinIcon { get; } = new("\uE77A"); // Unpin icon

    public static IconInfo PinIcon { get; } = new("\uE840"); // Pin icon

    public static IconInfo UninstallApplicationIcon { get; } = new("\uE7B8"); // Uninstall icon
}


Review comment (suggestion):
`InstallPackageListItem` in built-in WinGet extension has the same menu item but uses a different [icon](https://github.com/mKpwnz/PowerToys/blob/56aa9acfb405a36988584a46420f2793594b8f7a/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WinGet/Pages/InstallPackageListItem.cs#L193) (a trash can), which is consistent with Windows. 

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 Microsoft.CommandPalette.Extensions.Toolkit;

namespace Microsoft.CmdPal.Ext.Apps;

internal sealed class Icons
{
    internal static IconInfo AllAppsIcon => IconHelpers.FromRelativePath("Assets\\AllApps.svg");

    internal static IconInfo RunAsUserIcon => new("\uE7EE"); // OtherUser icon

    internal static IconInfo RunAsAdminIcon => new("\uE7EF"); // Admin icon

    internal static IconInfo OpenPathIcon => new("\ue838"); // Folder Open icon

    internal static IconInfo CopyIcon => new("\ue8c8"); // Copy icon

    public static IconInfo UnpinIcon { get; } = new("\uE77A"); // Unpin icon

    public static IconInfo PinIcon { get; } = new("\uE840"); // Pin icon

    public static IconInfo UninstallApplicationIcon { get; } = new("\uE74D"); // Uninstall icon
}
Repository: microsoft/PowerToys
File: src/modules/cmdpal/Microsoft.CmdPal.Core.ViewModels/ExtensionObjectViewModel.cs (C#)

Code before:
            SafeInitializePropertiesSynchronous();
        });
        t.Start();
        await t;
    }

    public void SafeInitializePropertiesSynchronous()
    {
        try
        {
            InitializeProperties();
        }
        catch (Exception ex)
        {
            ShowException(ex);
        }
    }

    public abstract void InitializeProperties();

    protected void UpdateProperty(string propertyName)
    {
        DoOnUiThread(() => OnPropertyChanged(propertyName));
    }

    protected void ShowException(Exception ex, string? extensionHint = null)
    {
        if (PageContext.TryGetTarget(out var pageContext))
        {
            pageContext.ShowException(ex, extensionHint);
        }
    }

    protected void DoOnUiThread(Action action)
    {
        if (PageContext.TryGetTarget(out var pageContext))
        {
            Task.Factory.StartNew(
                action,
                CancellationToken.None,
                TaskCreationOptions.None,
                pageContext.Scheduler);
        }
    }

    protected virtual void UnsafeCleanup()
    {
        // base doesn't do anything, but sub-classes should override this.
    }

    public virtual void SafeCleanup()

Review comment (suggestion):
I don't know how I feel about these. Part of me says "oh, that's handy." Then I look at the code below that actually uses it and I say "eh, that's definitely not as easy to read as Calling UpdateProperty twice"

Code after:
            SafeInitializePropertiesSynchronous();
        });
        t.Start();
        await t;
    }

    public void SafeInitializePropertiesSynchronous()
    {
        try
        {
            InitializeProperties();
        }
        catch (Exception ex)
        {
            ShowException(ex);
        }
    }

    public abstract void InitializeProperties();

    protected void UpdateProperty(string propertyName)
    {
        DoOnUiThread(() => OnPropertyChanged(propertyName));
    }

    protected void UpdateProperty(string propertyName1, string propertyName2)
    {
        DoOnUiThread(() =>
        {
            OnPropertyChanged(propertyName1);
            OnPropertyChanged(propertyName2);
        });
    }

    protected void UpdateProperty(string propertyName1, string propertyName2, string propertyName3)
    {
        DoOnUiThread(() =>
        {
            OnPropertyChanged(propertyName1);
            OnPropertyChanged(propertyName2);
            OnPropertyChanged(propertyName3);
        });
    }

    protected void UpdateProperty(params string[] propertyNames)
    {
        DoOnUiThread(() =>
        {
            foreach (var propertyName in propertyNames)
            {
                OnPropertyChanged(propertyName);
Repository: microsoft/PowerToys
File: src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WindowWalker/Helpers/NativeMethods.cs (C#)

Code before:
    public const int ICON_BIG = 1;

    /// <summary>
    /// Retrieve the small icon for the window.
    /// </summary>
    public const int ICON_SMALL = 0;

    /// <summary>
    /// Retrieve the small icon provided by the application.
    /// </summary>
    public const int ICON_SMALL2 = 2;

    /// <summary>
    /// The function returns if the receiving thread does not respond within the timeout period.
    /// </summary>
    public const int SMTO_ABORTIFHUNG = 0x0002;

    /// <summary>
    /// Retrieves a handle to the icon associated with the class.
    /// </summary>
    public const int GCLP_HICON = -14;

    /// <summary>
    /// Retrieves a handle to the small icon associated with the class.
    /// </summary>
    public const int GCLP_HICON_SM = -34;

    /// <summary>
    /// RPC call succeeded
    /// </summary>
    public const int RPC_S_OK = 0;

    /// <summary>
    /// The UUID is guaranteed to be unique to this computer only.
    /// </summary>
    public const int RPC_S_UUID_LOCAL_ONLY = 0x720;
}

public enum HRESULT : uint
{
    /// <summary>
    /// Operation successful.
    /// </summary>
    S_OK = 0x00000000,

    /// <summary>
    /// Operation successful. (negative condition/no operation)
    /// </summary>
    S_FALSE = 0x00000001,

    /// <summary>

Review comment (suggestion):
[nit] `GCLP_HICONSM` was a better name, as it reflects the Win32 constant name and makes it easier to locate

Code after:
    /// Retrieve the small icon for the window.
    /// </summary>
    public const int ICON_SMALL = 0;

    /// <summary>
    /// Retrieve the small icon provided by the application.
    /// </summary>
    public const int ICON_SMALL2 = 2;

    /// <summary>
    /// The function returns if the receiving thread does not respond within the timeout period.
    /// </summary>
    public const int SMTO_ABORTIFHUNG = 0x0002;

    /// <summary>
    /// Retrieves a handle to the icon associated with the class.
    /// </summary>
    public const int GCLP_HICON = -14;

    /// <summary>
    /// Retrieves a handle to the small icon associated with the class.
    /// </summary>
    public const int GCLP_HICONSM = -34;

    /// <summary>
    /// RPC call succeeded
    /// </summary>
    public const int RPC_S_OK = 0;

    /// <summary>
    /// The UUID is guaranteed to be unique to this computer only.
    /// </summary>
    public const int RPC_S_UUID_LOCAL_ONLY = 0x720;
}

public enum HRESULT : uint
{
    /// <summary>
    /// Operation successful.
    /// </summary>
    S_OK = 0x00000000,

    /// <summary>
    /// Operation successful. (negative condition/no operation)
    /// </summary>
    S_FALSE = 0x00000001,

    /// <summary>
    /// Not implemented.
    /// </summary>
    E_NOTIMPL = 0x80004001,