From 3c629bd1f37098888a9b8ecf7f06ba1447c6fc85 Mon Sep 17 00:00:00 2001
From: Andrew Whitechapel <andreww-msft@outlook.com>
Date: Mon, 20 May 2024 10:47:06 -0700
Subject: [PATCH 1/3] Enable the user to unpin/pin an external tool on the bar

---
 tools/PI/DevHome.PI/BarWindow.xaml            |   9 +-
 tools/PI/DevHome.PI/BarWindow.xaml.cs         |  66 ++++---
 tools/PI/DevHome.PI/Helpers/ExternalTool.cs   |  20 +-
 .../DevHome.PI/Helpers/ExternalToolsHelper.cs | 174 +++++++++++++++---
 tools/PI/DevHome.PI/Models/PerfCounters.cs    |  20 +-
 .../DevHome.PI/SettingsUi/AddToolControl.xaml |   9 +-
 .../SettingsUi/AddToolControl.xaml.cs         |  15 +-
 .../SettingsUi/EditToolsControl.xaml          |   2 +-
 .../SettingsUi/EditToolsControl.xaml.cs       |   2 +-
 .../DevHome.PI/Strings/en-us/Resources.resw   |  18 +-
 10 files changed, 254 insertions(+), 81 deletions(-)

diff --git a/tools/PI/DevHome.PI/BarWindow.xaml b/tools/PI/DevHome.PI/BarWindow.xaml
index 877679fbbe..5f8100509a 100644
--- a/tools/PI/DevHome.PI/BarWindow.xaml
+++ b/tools/PI/DevHome.PI/BarWindow.xaml
@@ -93,13 +93,10 @@
                     <!-- External tools -->
                     <StackPanel Orientation="Horizontal">
                         <StackPanel.ContextFlyout>
-                            <!-- TODO Re-enable this when we have Click handler implementation.-->
                             <MenuFlyout x:Name="ToolContextMenu">
                                 <MenuFlyoutItem
-                                    x:Uid="UnPinMenuItem"
-                                    x:Name="UnPinMenuItem"
-                                    Click="UnPinMenuItem_Click"
-                                    IsEnabled="False">
+                                    x:Name="PinUnpinMenuItem"
+                                    Click="PinUnpinMenuItem_Click">
                                     <MenuFlyoutItem.Icon>
                                         <FontIcon Glyph="&#xE77A;"/>
                                     </MenuFlyoutItem.Icon>
@@ -131,7 +128,7 @@
 
                         <ItemsRepeater
                             x:Name="ExternalToolsRepeater"
-                            ItemsSource="{x:Bind helpers:ExternalToolsHelper.Instance.ExternalTools, Mode=OneWay}"
+                            ItemsSource="{x:Bind helpers:ExternalToolsHelper.Instance.FilteredExternalTools, Mode=OneWay}"
                             Layout="{StaticResource ExternalToolsHorizontalLayout}">
                             <DataTemplate x:DataType="helpers:ExternalTool">
                                 <Button
diff --git a/tools/PI/DevHome.PI/BarWindow.xaml.cs b/tools/PI/DevHome.PI/BarWindow.xaml.cs
index cdeb5d8a8a..4695246b89 100644
--- a/tools/PI/DevHome.PI/BarWindow.xaml.cs
+++ b/tools/PI/DevHome.PI/BarWindow.xaml.cs
@@ -19,6 +19,7 @@
 using Microsoft.UI.Windowing;
 using Microsoft.UI.Xaml;
 using Microsoft.UI.Xaml.Controls;
+using Microsoft.UI.Xaml.Data;
 using Microsoft.UI.Xaml.Input;
 using Windows.UI.WindowManagement;
 using Windows.Win32;
@@ -36,6 +37,8 @@ public partial class BarWindow : WindowEx, INotifyPropertyChanged
     private readonly Settings settings = Settings.Default;
     private readonly string errorTitleText = CommonHelper.GetLocalizedString("ToolLaunchErrorTitle");
     private readonly string errorMessageText = CommonHelper.GetLocalizedString("ToolLaunchErrorMessage");
+    private readonly string pinMenuItemText = CommonHelper.GetLocalizedString("PinMenuItemText");
+    private readonly string unpinMenuItemText = CommonHelper.GetLocalizedString("UnpinMenuItemText");
     private readonly BarWindowViewModel viewModel = new();
 
     // Constants that control window sizes
@@ -138,12 +141,10 @@ private bool IsMaximized
     private readonly WINEVENTPROC winPositionEventDelegate;
     private readonly WINEVENTPROC winFocusEventDelegate;
 
-    private Button? selectedExternalToolButton;
-    private MenuFlyoutItem? selectedExternalToolMenuItem;
-
     private HWINEVENTHOOK positionEventHook;
     private HWINEVENTHOOK focusEventHook;
 
+    private ExternalTool? selectedExternalTool;
     private INotifyCollectionChanged? externalTools;
 
     internal static HWND ThisHwnd { get; private set; }
@@ -234,14 +235,15 @@ private void InitializeExternalTools()
     {
         ExternalToolsHelper.Instance.Init();
 
-        foreach (var item in ExternalToolsHelper.Instance.ExternalTools)
+        ExternalToolsMenu.Items.Clear();
+        foreach (var item in ExternalToolsHelper.Instance.AllExternalTools)
         {
             CreateMenuItemFromTool(item);
         }
 
         // We have to cast to INotifyCollectionChanged explicitly because the CollectionChanged
         // event in ReadOnlyObservableCollection is protected.
-        externalTools = ExternalToolsHelper.Instance.ExternalTools;
+        externalTools = ExternalToolsHelper.Instance.AllExternalTools;
         externalTools.CollectionChanged += ExternalTools_CollectionChanged;
     }
 
@@ -281,7 +283,7 @@ private void CreateMenuItemFromTool(ExternalTool item)
             Icon = item.MenuIcon,
         };
         menuItem.Click += ExternalToolMenuItem_Click;
-        menuItem.RightTapped += MenuItem_RightTapped;
+        menuItem.RightTapped += ExternalToolMenuItem_RightTapped;
         ExternalToolsMenu.Items.Add(menuItem);
 
         // You can't databind to MenuFlyoutItem, and the ExternalTool icon image is generated asynchronously,
@@ -308,12 +310,21 @@ private void ManageExternalToolsButton_Click(object sender, RoutedEventArgs e)
         settingsTool.Show();
     }
 
-    private void MenuItem_RightTapped(object sender, RightTappedRoutedEventArgs e)
+    private void ExternalToolMenuItem_RightTapped(object sender, RightTappedRoutedEventArgs e)
     {
         var menuItem = sender as MenuFlyoutItem;
         if (menuItem is not null)
         {
-            selectedExternalToolMenuItem = menuItem;
+            selectedExternalTool = (ExternalTool)menuItem.Tag;
+            if (selectedExternalTool.IsPinned)
+            {
+                PinUnpinMenuItem.Text = unpinMenuItemText;
+            }
+            else
+            {
+                PinUnpinMenuItem.Text = pinMenuItemText;
+            }
+
             ToolContextMenu.ShowAt(menuItem, e.GetPosition(menuItem));
         }
     }
@@ -357,36 +368,35 @@ private void InvokeTool(ExternalTool tool, int? id, HWND hWnd)
 
     private void ExternalToolButton_PointerPressed(object sender, PointerRoutedEventArgs e)
     {
-        selectedExternalToolButton = (Button)sender;
+        if (sender is Button clickedButton)
+        {
+            selectedExternalTool = (ExternalTool)clickedButton.Tag;
+            if (selectedExternalTool.IsPinned)
+            {
+                PinUnpinMenuItem.Text = unpinMenuItemText;
+            }
+            else
+            {
+                PinUnpinMenuItem.Text = pinMenuItemText;
+            }
+        }
     }
 
-    private void UnPinMenuItem_Click(object sender, RoutedEventArgs e)
+    private void PinUnpinMenuItem_Click(object sender, RoutedEventArgs e)
     {
-        // TODO Unpin the tool from the bar, but don't remove it from the collection,
-        // so it stays registered, and it stays in the ExternalTools menu.
-        if (selectedExternalToolButton is not null)
-        {
-        }
-        else if (selectedExternalToolMenuItem is not null)
+        // Pin or unpin the tool on the bar.
+        if (selectedExternalTool is not null)
         {
+            selectedExternalTool.IsPinned = !selectedExternalTool.IsPinned;
         }
     }
 
     private void UnregisterMenuItem_Click(object sender, RoutedEventArgs e)
     {
-        if (selectedExternalToolButton is not null)
+        if (selectedExternalTool is not null)
         {
-            if (selectedExternalToolButton.Tag is ExternalTool tool)
-            {
-                ExternalToolsHelper.Instance.RemoveExternalTool(tool);
-            }
-        }
-        else if (selectedExternalToolMenuItem is not null)
-        {
-            if (selectedExternalToolMenuItem.Tag is ExternalTool tool)
-            {
-                ExternalToolsHelper.Instance.RemoveExternalTool(tool);
-            }
+            ExternalToolsHelper.Instance.RemoveExternalTool(selectedExternalTool);
+            selectedExternalTool = null;
         }
     }
 
diff --git a/tools/PI/DevHome.PI/Helpers/ExternalTool.cs b/tools/PI/DevHome.PI/Helpers/ExternalTool.cs
index d169725875..0a189838b5 100644
--- a/tools/PI/DevHome.PI/Helpers/ExternalTool.cs
+++ b/tools/PI/DevHome.PI/Helpers/ExternalTool.cs
@@ -44,7 +44,19 @@ public string OtherArgs
     {
         get; private set;
     }
-
+
+    private bool isPinned;
+
+    public bool IsPinned
+    {
+        get => isPinned;
+        set
+        {
+            isPinned = value;
+            OnPropertyChanged(nameof(IsPinned));
+        }
+    }
+
     [JsonIgnore]
     private SoftwareBitmapSource? _toolIcon;
 
@@ -81,13 +93,15 @@ public ExternalTool(
         string executable,
         ExternalToolArgType argtype,
         string argprefix = "",
-        string otherArgs = "")
+        string otherArgs = "",
+        bool isPinned = false)
     {
         Name = name;
         Executable = executable;
         ArgType = argtype;
         ArgPrefix = argprefix;
-        OtherArgs = otherArgs;
+        OtherArgs = otherArgs;
+        IsPinned = isPinned;
 
         ID = Guid.NewGuid().ToString();
 
diff --git a/tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs b/tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs
index e410984690..128caf2d85 100644
--- a/tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs
+++ b/tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs
@@ -3,6 +3,8 @@
 
 using System;
 using System.Collections.ObjectModel;
+using System.Collections.Specialized;
+using System.ComponentModel;
 using System.IO;
 using System.Reflection;
 using System.Text.Json;
@@ -15,14 +17,44 @@ namespace DevHome.PI.Helpers;
 internal sealed class ExternalToolsHelper
 {
     private readonly JsonSerializerOptions serializerOptions = new() { WriteIndented = true };
-    private readonly ObservableCollection<ExternalTool> externalTools = [];
     private readonly string toolInfoFileName;
 
     public static readonly ExternalToolsHelper Instance = new();
 
     private static readonly ILogger _log = Log.ForContext("SourceContext", nameof(ExternalToolsHelper));
 
-    public ReadOnlyObservableCollection<ExternalTool> ExternalTools { get; set; }
+    private ObservableCollection<ExternalTool> allExternalTools = [];
+
+    // The ExternalTools menu shows all registered tools.
+    public ObservableCollection<ExternalTool> AllExternalTools
+    {
+        get => allExternalTools;
+        set
+        {
+            // We're assigning the collection once, and this also covers the case where we reassign it again:
+            // we need to unsubscribe from the old collection's events, subscribe to the new collection's events,
+            // and initialize the filtered collection.
+            if (allExternalTools != value)
+            {
+                if (allExternalTools != null)
+                {
+                    allExternalTools.CollectionChanged -= AllExternalTools_CollectionChanged;
+                }
+
+                allExternalTools = value;
+                if (allExternalTools != null)
+                {
+                    allExternalTools.CollectionChanged += AllExternalTools_CollectionChanged;
+                }
+
+                // Synchronize the filtered collection with this unfiltered one.
+                SynchronizeAllFilteredItems();
+            }
+        }
+    }
+
+    // The bar shows only the pinned tools.
+    public ObservableCollection<ExternalTool> FilteredExternalTools { get; private set; } = [];
 
     private ExternalToolsHelper()
     {
@@ -36,37 +68,75 @@ private ExternalToolsHelper()
             localFolder = Path.GetDirectoryName(Assembly.GetEntryAssembly()?.Location) ?? string.Empty;
         }
 
+        // The file should be in this location:
+        // %LocalAppData%\Packages\Microsoft.Windows.DevHome.Dev_8wekyb3d8bbwe\LocalState\externaltools.json
         toolInfoFileName = Path.Combine(localFolder, "externaltools.json");
-        ExternalTools = new(externalTools);
+        AllExternalTools = new(allExternalTools);
     }
 
     internal void Init()
     {
+        allExternalTools.Clear();
         if (File.Exists(toolInfoFileName))
         {
             try
             {
                 var jsonData = File.ReadAllText(toolInfoFileName);
                 var existingData = JsonSerializer.Deserialize<ExternalTool[]>(jsonData) ?? [];
-                foreach (var data in existingData)
+                foreach (var toolItem in existingData)
                 {
-                    externalTools.Add(data);
+                    allExternalTools.Add(toolItem);
+                    toolItem.PropertyChanged += ToolItem_PropertyChanged;
                 }
             }
             catch (Exception ex)
             {
-                // TODO If we failed parsing the JSON file... should we just delete it?
+                // TODO If we failed to parse the JSON file, we should rename it (using DateTime.Now),
+                // create a new one, and report to the user.
                 _log.Error(ex, "Failed to parse {tool}", toolInfoFileName);
             }
         }
     }
 
+    private void ToolItem_PropertyChanged(object? sender, PropertyChangedEventArgs e)
+    {
+        // The user can change the IsPinned property of a tool, to pin or unpin it on the bar.
+        if (sender is ExternalTool tool && e.PropertyName == nameof(ExternalTool.IsPinned))
+        {
+            if (tool.IsPinned)
+            {
+                if (!FilteredExternalTools.Contains(tool))
+                {
+                    FilteredExternalTools.Add(tool);
+                }
+            }
+            else
+            {
+                FilteredExternalTools.Remove(tool);
+            }
+        }
+
+        WriteToolsJsonFile();
+    }
+
     public ExternalTool AddExternalTool(ExternalTool tool)
     {
-        externalTools.Add(tool);
+        allExternalTools.Add(tool);
+        WriteToolsJsonFile();
+        return tool;
+    }
 
-        // Write out to JSON file
-        var updatedJson = JsonSerializer.Serialize(externalTools, serializerOptions);
+    public void RemoveExternalTool(ExternalTool tool)
+    {
+        if (allExternalTools.Remove(tool))
+        {
+            WriteToolsJsonFile();
+        }
+    }
+
+    private void WriteToolsJsonFile()
+    {
+        var updatedJson = JsonSerializer.Serialize(allExternalTools, serializerOptions);
 
         try
         {
@@ -74,27 +144,85 @@ public ExternalTool AddExternalTool(ExternalTool tool)
         }
         catch (Exception ex)
         {
-            // TODO What should we do if we're unable to write to the file?
-            _log.Error(ex, "AddExternalTool unable to write to file");
+            // TODO If we're unable to write to the file, we should figure out why.
+            // If the file has become corrupted, we should rename it (using DateTime.Now),
+            // create a new one, and report to the user. If it's locked, we just report to the user.
+            _log.Error(ex, "WriteToolsJsonFile unable to write to file");
         }
-
-        return tool;
     }
 
-    public void RemoveExternalTool(ExternalTool tool)
+    private void AllExternalTools_CollectionChanged(object? sender, NotifyCollectionChangedEventArgs e)
     {
-        if (externalTools.Remove(tool))
+        // Whenever the "all tools" collection changes, we need to synchronize the filtered collection.
+        switch (e.Action)
         {
-            // Write out to JSON file
-            var updatedJson = JsonSerializer.Serialize(externalTools, serializerOptions);
+            case NotifyCollectionChangedAction.Add:
+                if (e.NewItems is not null)
+                {
+                    foreach (ExternalTool newItem in e.NewItems)
+                    {
+                        if (newItem.IsPinned)
+                        {
+                            FilteredExternalTools.Add(newItem);
+                        }
 
-            try
-            {
-                File.WriteAllText(toolInfoFileName, updatedJson);
-            }
-            catch (Exception ex)
+                        newItem.PropertyChanged += ToolItem_PropertyChanged;
+                    }
+                }
+
+                break;
+
+            case NotifyCollectionChangedAction.Remove:
+                if (e.OldItems is not null)
+                {
+                    foreach (ExternalTool oldItem in e.OldItems)
+                    {
+                        oldItem.PropertyChanged -= ToolItem_PropertyChanged;
+                        FilteredExternalTools.Remove(oldItem);
+                    }
+                }
+
+                break;
+
+            case NotifyCollectionChangedAction.Replace:
+                if (e.OldItems is not null)
+                {
+                    foreach (ExternalTool oldItem in e.OldItems)
+                    {
+                        oldItem.PropertyChanged -= ToolItem_PropertyChanged;
+                        FilteredExternalTools.Remove(oldItem);
+                    }
+                }
+
+                if (e.NewItems is not null)
+                {
+                    foreach (ExternalTool newItem in e.NewItems)
+                    {
+                        if (newItem.IsPinned)
+                        {
+                            FilteredExternalTools.Add(newItem);
+                        }
+
+                        newItem.PropertyChanged += ToolItem_PropertyChanged;
+                    }
+                }
+
+                break;
+
+            case NotifyCollectionChangedAction.Reset:
+                SynchronizeAllFilteredItems();
+                break;
+        }
+    }
+
+    private void SynchronizeAllFilteredItems()
+    {
+        FilteredExternalTools.Clear();
+        foreach (var item in AllExternalTools)
+        {
+            if (item.IsPinned)
             {
-                _log.Error(ex, "RemoveExternalTool unable to write to file");
+                FilteredExternalTools.Add(item);
             }
         }
     }
diff --git a/tools/PI/DevHome.PI/Models/PerfCounters.cs b/tools/PI/DevHome.PI/Models/PerfCounters.cs
index 7edc33b47e..a5311ee387 100644
--- a/tools/PI/DevHome.PI/Models/PerfCounters.cs
+++ b/tools/PI/DevHome.PI/Models/PerfCounters.cs
@@ -88,10 +88,10 @@ private void TargetApp_PropertyChanged(object? sender, PropertyChangedEventArgs
         if (e.PropertyName == nameof(TargetAppData.TargetProcess))
         {
             ThreadPool.QueueUserWorkItem((o) => UpdateTargetProcess(TargetAppData.Instance.TargetProcess));
-        }
-        else if (e.PropertyName == nameof(TargetAppData.HasExited))
-        {
-            CloseTargetCounters();
+        }
+        else if (e.PropertyName == nameof(TargetAppData.HasExited))
+        {
+            CloseTargetCounters();
         }
     }
 
@@ -134,25 +134,25 @@ public void Stop()
     private void CloseTargetCounters()
     {
         cpuCounter?.Close();
-        cpuCounter?.Dispose();
+        cpuCounter?.Dispose();
         cpuCounter = null;
         ramCounter?.Close();
-        ramCounter?.Dispose();
+        ramCounter?.Dispose();
         ramCounter = null;
 
         foreach (var counter in gpuCounters ?? Enumerable.Empty<PerformanceCounter>())
         {
             counter.Close();
             counter.Dispose();
-        }
-
+        }
+
         gpuCounters?.Clear();
 
         readCounter?.Close();
-        readCounter?.Dispose();
+        readCounter?.Dispose();
         readCounter = null;
         writeCounter?.Close();
-        writeCounter?.Dispose();
+        writeCounter?.Dispose();
         writeCounter = null;
     }
 
diff --git a/tools/PI/DevHome.PI/SettingsUi/AddToolControl.xaml b/tools/PI/DevHome.PI/SettingsUi/AddToolControl.xaml
index ec22882a7d..db614c1b8c 100644
--- a/tools/PI/DevHome.PI/SettingsUi/AddToolControl.xaml
+++ b/tools/PI/DevHome.PI/SettingsUi/AddToolControl.xaml
@@ -14,6 +14,7 @@
             <RowDefinition Height="Auto"/>
             <RowDefinition Height="Auto"/>
             <RowDefinition Height="Auto"/>
+            <RowDefinition Height="Auto"/>
         </Grid.RowDefinitions>
 
         <Border Margin="0">
@@ -92,8 +93,12 @@
             </Grid>
         </Border>
 
-        <StackPanel Orientation="Horizontal" Grid.Row="4" HorizontalAlignment="Right" Margin="0,12,0,0">
-            <Button x:Uid="AddToolsOkButton" x:Name="OkButton"  Click="OkButton_Click" Width="100"/>
+        <ToggleSwitch 
+            x:Uid="IsPinnedToggleSwitch" x:Name="IsPinnedToggleSwitch"
+            Grid.Row="4" Margin="0,12,0,0" HorizontalAlignment="Left" IsOn="True"/>
+        
+        <StackPanel Orientation="Horizontal" Grid.Row="5" HorizontalAlignment="Right" Margin="0,12,0,0">
+            <Button x:Uid="AddToolsOkButton" x:Name="OkButton" Click="OkButton_Click" Width="100"/>
             <Button x:Uid="AddToolsCancelButton" x:Name="CancelButton" Margin="6,0,0,0" Width="100"/>
         </StackPanel>
     </Grid>
diff --git a/tools/PI/DevHome.PI/SettingsUi/AddToolControl.xaml.cs b/tools/PI/DevHome.PI/SettingsUi/AddToolControl.xaml.cs
index 1d20ec496c..b4437475ef 100644
--- a/tools/PI/DevHome.PI/SettingsUi/AddToolControl.xaml.cs
+++ b/tools/PI/DevHome.PI/SettingsUi/AddToolControl.xaml.cs
@@ -76,22 +76,29 @@ private void ClearValues()
         NoneRadio.IsChecked = true;
         PrefixTextBox.Text = string.Empty;
         OtherArgsTextBox.Text = string.Empty;
+        IsPinnedToggleSwitch.IsOn = true;
     }
 
     private ExternalTool GetCurrentToolDefinition()
     {
         var argType = ExternalToolArgType.None;
 
-        if (HwndRadio?.IsChecked ?? false)
+        if (HwndRadio.IsChecked ?? false)
         {
             argType = ExternalToolArgType.Hwnd;
         }
-        else if (ProcessIdRadio?.IsChecked ?? false)
+        else if (ProcessIdRadio.IsChecked ?? false)
         {
             argType = ExternalToolArgType.ProcessId;
         }
 
-        return new(ToolNameTextBox.Text, ToolPathTextBox.Text, argType, PrefixTextBox?.Text ?? string.Empty, OtherArgsTextBox?.Text ?? string.Empty);
+        return new(
+            ToolNameTextBox.Text,
+            ToolPathTextBox.Text,
+            argType,
+            PrefixTextBox.Text ?? string.Empty,
+            OtherArgsTextBox.Text ?? string.Empty,
+            IsPinnedToggleSwitch.IsOn);
     }
 
     private void UpdateSampleCommandline(object sender, TextChangedEventArgs e)
@@ -106,7 +113,7 @@ private void UpdateSampleCommandline(object sender, RoutedEventArgs e)
 
     private void UpdateSampleCommandline()
     {
-        if (SampleCommandTextBox == null)
+        if (SampleCommandTextBox is null)
         {
             // The window is still initializing.
             return;
diff --git a/tools/PI/DevHome.PI/SettingsUi/EditToolsControl.xaml b/tools/PI/DevHome.PI/SettingsUi/EditToolsControl.xaml
index edd250b572..d6f6016464 100644
--- a/tools/PI/DevHome.PI/SettingsUi/EditToolsControl.xaml
+++ b/tools/PI/DevHome.PI/SettingsUi/EditToolsControl.xaml
@@ -32,7 +32,7 @@
             GridLinesVisibility="None"
             HeadersVisibility="Column">
             <controls:DataGrid.Columns>
-                <controls:DataGridTextColumn x:Uid="EditToolsNameColumn" Binding="{Binding Name}" Width="90"/>
+                <controls:DataGridTextColumn x:Uid="EditToolsNameColumn" Binding="{Binding Name}" Width="150"/>
             </controls:DataGrid.Columns>
         </controls:DataGrid>
 
diff --git a/tools/PI/DevHome.PI/SettingsUi/EditToolsControl.xaml.cs b/tools/PI/DevHome.PI/SettingsUi/EditToolsControl.xaml.cs
index e40dc1ac3d..a3b3153687 100644
--- a/tools/PI/DevHome.PI/SettingsUi/EditToolsControl.xaml.cs
+++ b/tools/PI/DevHome.PI/SettingsUi/EditToolsControl.xaml.cs
@@ -12,7 +12,7 @@ public sealed partial class EditToolsControl : UserControl
     public EditToolsControl()
     {
         InitializeComponent();
-        ToolsDataGrid.ItemsSource = ExternalToolsHelper.Instance.ExternalTools;
+        ToolsDataGrid.ItemsSource = ExternalToolsHelper.Instance.AllExternalTools;
     }
 
     private void DeleteToolButton_Click(object sender, RoutedEventArgs e)
diff --git a/tools/PI/DevHome.PI/Strings/en-us/Resources.resw b/tools/PI/DevHome.PI/Strings/en-us/Resources.resw
index 519f4b00c0..62849471c5 100644
--- a/tools/PI/DevHome.PI/Strings/en-us/Resources.resw
+++ b/tools/PI/DevHome.PI/Strings/en-us/Resources.resw
@@ -137,9 +137,9 @@
     <value>Close the PI window and the service</value>
     <comment>The tooltip for the close all menu item flyout. This will cause the program to terminate.</comment>
   </data>
-  <data name="UnPinMenuItem.Text" xml:space="preserve">
-    <value>Unpin</value>
-    <comment>The label for Unpin menu flyout for an icon in the toolbar. This will cause the icon to no longer be pinned to the bar.</comment>
+  <data name="PinMenuItemText" xml:space="preserve">
+    <value>Pin</value>
+    <comment>A label for the menuitem to pin a tool on the bar.</comment>
   </data>
   <data name="UnregisterMenuItem.Text" xml:space="preserve">
     <value>Unregister</value>
@@ -757,4 +757,16 @@
     <value>Entrypoint address</value>
     <comment>The contents of a text block referring to the entrypoint address of a loaded module.</comment>
   </data>
+  <data name="IsPinnedToggleSwitch.OnContent" xml:space="preserve">
+    <value>Pinned</value>
+    <comment>Text to show when the switch is turned on, to pin the tool to the bar.</comment>
+  </data>
+  <data name="IsPinnedToggleSwitch.OffContent" xml:space="preserve">
+    <value>Not pinned</value>
+    <comment>Text to show when the switch is turned off, to not pin the tool to the bar.</comment>
+  </data>
+  <data name="UnpinMenuItemText" xml:space="preserve">
+    <value>Unpin</value>
+    <comment>A label for the menuitem to unpin a tool on the bar.</comment>
+  </data>
 </root>
\ No newline at end of file

From a8399fb0f17ef48948671ccd781c842ca2db8477 Mon Sep 17 00:00:00 2001
From: Andrew Whitechapel <andreww-msft@outlook.com>
Date: Mon, 20 May 2024 16:19:05 -0700
Subject: [PATCH 2/3] Updates per feedback

---
 tools/PI/DevHome.PI/BarWindow.xaml            |  4 +-
 tools/PI/DevHome.PI/BarWindow.xaml.cs         |  2 +-
 tools/PI/DevHome.PI/Helpers/ExternalTool.cs   | 66 +++++--------------
 .../DevHome.PI/Helpers/ExternalToolsHelper.cs |  6 +-
 .../DevHome.PI/SettingsUi/AddToolControl.xaml |  6 +-
 5 files changed, 25 insertions(+), 59 deletions(-)

diff --git a/tools/PI/DevHome.PI/BarWindow.xaml b/tools/PI/DevHome.PI/BarWindow.xaml
index 5f8100509a..147f5289ba 100644
--- a/tools/PI/DevHome.PI/BarWindow.xaml
+++ b/tools/PI/DevHome.PI/BarWindow.xaml
@@ -133,11 +133,11 @@
                             <DataTemplate x:DataType="helpers:ExternalTool">
                                 <Button
                                     Click="ExternalToolButton_Click"
-                                    HorizontalAlignment="Center" Width="45" Height="45"
+                                    HorizontalAlignment="Center" 
                                     ToolTipService.ToolTip="{x:Bind Name, Mode=OneWay}"
                                     Tag="{x:Bind}"
                                     PointerPressed="ExternalToolButton_PointerPressed">
-                                    <Image Source="{x:Bind ToolIcon, Mode=OneWay}"/>
+                                    <Image Source="{x:Bind ToolIcon, Mode=OneWay}" Width="26" Height="26"/>
                                 </Button>
                             </DataTemplate>
                         </ItemsRepeater>
diff --git a/tools/PI/DevHome.PI/BarWindow.xaml.cs b/tools/PI/DevHome.PI/BarWindow.xaml.cs
index 4695246b89..27a8a08a17 100644
--- a/tools/PI/DevHome.PI/BarWindow.xaml.cs
+++ b/tools/PI/DevHome.PI/BarWindow.xaml.cs
@@ -294,7 +294,7 @@ private void CreateMenuItemFromTool(ExternalTool item)
 
     private void ExternalToolItem_PropertyChanged(object? sender, PropertyChangedEventArgs e)
     {
-        if (sender is ExternalTool item && e.PropertyName == nameof(ExternalTool.MenuIcon))
+        if (sender is ExternalTool item && string.Equals(e.PropertyName, nameof(ExternalTool.MenuIcon), StringComparison.Ordinal))
         {
             var menuItem = (MenuFlyoutItem?)ExternalToolsMenu.Items.FirstOrDefault(i => ((ExternalTool)i.Tag).ID == item.ID);
             if (menuItem is not null)
diff --git a/tools/PI/DevHome.PI/Helpers/ExternalTool.cs b/tools/PI/DevHome.PI/Helpers/ExternalTool.cs
index 0a189838b5..efcca40459 100644
--- a/tools/PI/DevHome.PI/Helpers/ExternalTool.cs
+++ b/tools/PI/DevHome.PI/Helpers/ExternalTool.cs
@@ -5,6 +5,7 @@
 using System.ComponentModel;
 using System.Diagnostics;
 using System.Text.Json.Serialization;
+using CommunityToolkit.Mvvm.ComponentModel;
 using Microsoft.UI.Xaml.Controls;
 using Microsoft.UI.Xaml.Media.Imaging;
 using Serilog;
@@ -22,7 +23,7 @@ public enum ExternalToolArgType
 }
 
 // ExternalTool represents an imported tool
-public class ExternalTool : INotifyPropertyChanged
+public partial class ExternalTool : ObservableObject
 {
     private static readonly ILogger _log = Log.ForContext("SourceContext", nameof(ExternalTool));
 
@@ -45,48 +46,20 @@ public string OtherArgs
         get; private set;
     }
 
-    private bool isPinned;
+    [ObservableProperty]
+    private bool _isPinned;
 
-    public bool IsPinned
-    {
-        get => isPinned;
-        set
-        {
-            isPinned = value;
-            OnPropertyChanged(nameof(IsPinned));
-        }
-    }
-
-    [JsonIgnore]
+    // Note the additional "property:" syntax to ensure the JsonIgnore is propagated to the generated property.
+    [ObservableProperty]
+    [property: JsonIgnore]
     private SoftwareBitmapSource? _toolIcon;
 
-    [JsonIgnore]
-    public SoftwareBitmapSource? ToolIcon
-    {
-        get => _toolIcon;
-        private set
-        {
-            _toolIcon = value;
-            OnPropertyChanged(nameof(ToolIcon));
-        }
-    }
-
-    [JsonIgnore]
+    [ObservableProperty]
+    [property: JsonIgnore]
     private BitmapIcon? _menuIcon;
 
     [JsonIgnore]
-    public BitmapIcon? MenuIcon
-    {
-        get => _menuIcon;
-        private set
-        {
-            _menuIcon = value;
-            OnPropertyChanged(nameof(MenuIcon));
-        }
-    }
-
-    [JsonIgnore]
-    private SoftwareBitmap? softwareBitmap;
+    private SoftwareBitmap? _softwareBitmap;
 
     public ExternalTool(
         string name,
@@ -116,10 +89,10 @@ private async void GetToolImage()
     {
         try
         {
-            softwareBitmap ??= GetSoftwareBitmapFromExecutable(Executable);
-            if (softwareBitmap is not null)
+            _softwareBitmap ??= GetSoftwareBitmapFromExecutable(Executable);
+            if (_softwareBitmap is not null)
             {
-                ToolIcon = await GetSoftwareBitmapSourceFromSoftwareBitmap(softwareBitmap);
+                ToolIcon = await GetSoftwareBitmapSourceFromSoftwareBitmap(_softwareBitmap);
             }
         }
         catch (Exception ex)
@@ -132,10 +105,10 @@ private async void GetMenuIcon()
     {
         try
         {
-            softwareBitmap ??= GetSoftwareBitmapFromExecutable(Executable);
-            if (softwareBitmap is not null)
+            _softwareBitmap ??= GetSoftwareBitmapFromExecutable(Executable);
+            if (_softwareBitmap is not null)
             {
-                var bitmapUri = await SaveSoftwareBitmapToTempFile(softwareBitmap);
+                var bitmapUri = await SaveSoftwareBitmapToTempFile(_softwareBitmap);
                 MenuIcon = new BitmapIcon
                 {
                     UriSource = bitmapUri,
@@ -188,11 +161,4 @@ internal string CreateCommandLine(int? pid, HWND? hwnd)
             return null;
         }
     }
-
-    public event PropertyChangedEventHandler? PropertyChanged;
-
-    protected void OnPropertyChanged(string propertyName)
-    {
-        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
-    }
 }
diff --git a/tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs b/tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs
index 128caf2d85..9bcd096bde 100644
--- a/tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs
+++ b/tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs
@@ -69,7 +69,7 @@ private ExternalToolsHelper()
         }
 
         // The file should be in this location:
-        // %LocalAppData%\Packages\Microsoft.Windows.DevHome.Dev_8wekyb3d8bbwe\LocalState\externaltools.json
+        // %LocalAppData%\Packages\Microsoft.Windows.DevHome_8wekyb3d8bbwe\LocalState\externaltools.json
         toolInfoFileName = Path.Combine(localFolder, "externaltools.json");
         AllExternalTools = new(allExternalTools);
     }
@@ -93,7 +93,7 @@ internal void Init()
             {
                 // TODO If we failed to parse the JSON file, we should rename it (using DateTime.Now),
                 // create a new one, and report to the user.
-                _log.Error(ex, "Failed to parse {tool}", toolInfoFileName);
+                _log.Error(ex, $"Failed to parse {toolInfoFileName}");
             }
         }
     }
@@ -101,7 +101,7 @@ internal void Init()
     private void ToolItem_PropertyChanged(object? sender, PropertyChangedEventArgs e)
     {
         // The user can change the IsPinned property of a tool, to pin or unpin it on the bar.
-        if (sender is ExternalTool tool && e.PropertyName == nameof(ExternalTool.IsPinned))
+        if (sender is ExternalTool tool && string.Equals(e.PropertyName, nameof(ExternalTool.IsPinned), StringComparison.Ordinal))
         {
             if (tool.IsPinned)
             {
diff --git a/tools/PI/DevHome.PI/SettingsUi/AddToolControl.xaml b/tools/PI/DevHome.PI/SettingsUi/AddToolControl.xaml
index db614c1b8c..16be0d9db3 100644
--- a/tools/PI/DevHome.PI/SettingsUi/AddToolControl.xaml
+++ b/tools/PI/DevHome.PI/SettingsUi/AddToolControl.xaml
@@ -97,9 +97,9 @@
             x:Uid="IsPinnedToggleSwitch" x:Name="IsPinnedToggleSwitch"
             Grid.Row="4" Margin="0,12,0,0" HorizontalAlignment="Left" IsOn="True"/>
         
-        <StackPanel Orientation="Horizontal" Grid.Row="5" HorizontalAlignment="Right" Margin="0,12,0,0">
-            <Button x:Uid="AddToolsOkButton" x:Name="OkButton" Click="OkButton_Click" Width="100"/>
-            <Button x:Uid="AddToolsCancelButton" x:Name="CancelButton" Margin="6,0,0,0" Width="100"/>
+        <StackPanel Orientation="Horizontal" Grid.Row="5" HorizontalAlignment="Right" Margin="0,12,0,0" Spacing="6">
+            <Button x:Uid="AddToolsOkButton" x:Name="OkButton" Click="OkButton_Click" MinWidth="100"/>
+            <Button x:Uid="AddToolsCancelButton" x:Name="CancelButton" MinWidth="100"/>
         </StackPanel>
     </Grid>
 </UserControl>

From 3adcb43bb3a24a7995fcffea574fc83b7005591a Mon Sep 17 00:00:00 2001
From: Andrew Whitechapel <andreww-msft@outlook.com>
Date: Tue, 21 May 2024 16:23:02 -0700
Subject: [PATCH 3/3] Made FilteredExternalTools a ReadOnlyObservableCollection

---
 .../DevHome.PI/Helpers/ExternalToolsHelper.cs | 23 +++++++++++--------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs b/tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs
index 9bcd096bde..02de51d806 100644
--- a/tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs
+++ b/tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs
@@ -23,6 +23,8 @@ internal sealed class ExternalToolsHelper
 
     private static readonly ILogger _log = Log.ForContext("SourceContext", nameof(ExternalToolsHelper));
 
+    private readonly ObservableCollection<ExternalTool> filteredExternalTools = [];
+
     private ObservableCollection<ExternalTool> allExternalTools = [];
 
     // The ExternalTools menu shows all registered tools.
@@ -54,7 +56,7 @@ public ObservableCollection<ExternalTool> AllExternalTools
     }
 
     // The bar shows only the pinned tools.
-    public ObservableCollection<ExternalTool> FilteredExternalTools { get; private set; } = [];
+    public ReadOnlyObservableCollection<ExternalTool> FilteredExternalTools { get; private set; }
 
     private ExternalToolsHelper()
     {
@@ -72,6 +74,7 @@ private ExternalToolsHelper()
         // %LocalAppData%\Packages\Microsoft.Windows.DevHome_8wekyb3d8bbwe\LocalState\externaltools.json
         toolInfoFileName = Path.Combine(localFolder, "externaltools.json");
         AllExternalTools = new(allExternalTools);
+        FilteredExternalTools = new(filteredExternalTools);
     }
 
     internal void Init()
@@ -105,14 +108,14 @@ private void ToolItem_PropertyChanged(object? sender, PropertyChangedEventArgs e
         {
             if (tool.IsPinned)
             {
-                if (!FilteredExternalTools.Contains(tool))
+                if (!filteredExternalTools.Contains(tool))
                 {
-                    FilteredExternalTools.Add(tool);
+                    filteredExternalTools.Add(tool);
                 }
             }
             else
             {
-                FilteredExternalTools.Remove(tool);
+                filteredExternalTools.Remove(tool);
             }
         }
 
@@ -163,7 +166,7 @@ private void AllExternalTools_CollectionChanged(object? sender, NotifyCollection
                     {
                         if (newItem.IsPinned)
                         {
-                            FilteredExternalTools.Add(newItem);
+                            filteredExternalTools.Add(newItem);
                         }
 
                         newItem.PropertyChanged += ToolItem_PropertyChanged;
@@ -178,7 +181,7 @@ private void AllExternalTools_CollectionChanged(object? sender, NotifyCollection
                     foreach (ExternalTool oldItem in e.OldItems)
                     {
                         oldItem.PropertyChanged -= ToolItem_PropertyChanged;
-                        FilteredExternalTools.Remove(oldItem);
+                        filteredExternalTools.Remove(oldItem);
                     }
                 }
 
@@ -190,7 +193,7 @@ private void AllExternalTools_CollectionChanged(object? sender, NotifyCollection
                     foreach (ExternalTool oldItem in e.OldItems)
                     {
                         oldItem.PropertyChanged -= ToolItem_PropertyChanged;
-                        FilteredExternalTools.Remove(oldItem);
+                        filteredExternalTools.Remove(oldItem);
                     }
                 }
 
@@ -200,7 +203,7 @@ private void AllExternalTools_CollectionChanged(object? sender, NotifyCollection
                     {
                         if (newItem.IsPinned)
                         {
-                            FilteredExternalTools.Add(newItem);
+                            filteredExternalTools.Add(newItem);
                         }
 
                         newItem.PropertyChanged += ToolItem_PropertyChanged;
@@ -217,12 +220,12 @@ private void AllExternalTools_CollectionChanged(object? sender, NotifyCollection
 
     private void SynchronizeAllFilteredItems()
     {
-        FilteredExternalTools.Clear();
+        filteredExternalTools.Clear();
         foreach (var item in AllExternalTools)
         {
             if (item.IsPinned)
             {
-                FilteredExternalTools.Add(item);
+                filteredExternalTools.Add(item);
             }
         }
     }