Enable the user to unpin/pin an external tool on the bar #2972

andreww-msft · 2024-05-20T17:51:27Z

Summary of the pull request

Prior to this, when the user added an external tool we always pinned it to the bar. Now, while we always add it to the menu, we let the user choose whether to pin it to the bar (defaults to pinned). This meant adding an IsPinned property to ExternalTool, a ToggleSwitch in the Settings | AddTool UI, adding a FilteredExternalTools collection which is synchronized with changes to the "all tools" collection, and databinding the buttons collection to this instead. The ExternalTools menu uses the "all tools" collection.

Fixed related bugs

when you add an external tool to the bar, and reload the bar, the tool is duped.
external tool menu should be restricted to external tool buttons, and not show up on the other bar items.
we save the list of tools to a file when a tool is added/removed, but we also need to do this when a tool property is changed.

tools/PI/DevHome.PI/Helpers/ExternalTool.cs

krschau · 2024-05-20T18:29:19Z

tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs

@@ -36,65 +68,161 @@ 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


Suggest not using Dev package path

Suggested change

// %LocalAppData%\Packages\Microsoft.Windows.DevHome.Dev_8wekyb3d8bbwe\LocalState\externaltools.json

// %LocalAppData%\Packages\Microsoft.Windows.DevHome_8wekyb3d8bbwe\LocalState\externaltools.json

Fixed in 2nd commit.

krschau · 2024-05-20T18:30:04Z

tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs

                }
            }
            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);


Suggested change

_log.Error(ex, "Failed to parse {tool}", toolInfoFileName);

_log.Error(ex, $"Failed to parse {toolInfoFileName}");

Fixed in 2nd commit.

krschau · 2024-05-20T18:30:37Z

tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs

+    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))


Prefer string.Equals to ==

Fixed in 2nd commit.

krschau · 2024-05-20T18:33:08Z

tools/PI/DevHome.PI/SettingsUi/AddToolControl.xaml

+            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"/>


Use MinWidth, otherwise you have a potential text scaling (or long language) accessibility bug.

Fixed in 2nd commit.

krschau · 2024-05-20T18:33:33Z

tools/PI/DevHome.PI/SettingsUi/AddToolControl.xaml

+            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"/>


Suggest adding Spacing to the surrounding StackPanel instead of Margin to the Button.

Fixed in 2nd commit.

timkur · 2024-05-21T19:31:02Z

tools/PI/DevHome.PI/BarWindow.xaml.cs

@@ -292,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))


==

We do this sort of comparison throughout a lot of the code. Should we be doing string.equals instead? #Resolved

So it seems, and yes I fixed this one in the 2nd commit.

timkur · 2024-05-21T20:41:29Z

tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs

+    }
+
+    // The bar shows only the pinned tools.
+    public ObservableCollection<ExternalTool> FilteredExternalTools { get; private set; } = [];


ObservableCollection

Is this a ReadOnly collection? #Resolved

No, I thought about wrapping it, but the set is private, and I need to be able to change at least the IsPinned property on any of the items in the collection.

ReadonlyCollection only prevents you from adding/removing from the collection, not modifying the items in the collection. I think you want this as a ReadOnly collection, as you don't want other components adding/removing from this collection, correct?

Fixed in 3rd commit.

timkur · 2024-05-21T20:44:27Z

tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs

+
+    private void SynchronizeAllFilteredItems()
+    {
+        FilteredExternalTools.Clear();


FilteredExternalTools.Clear();

I wonder if it would make sense to build a collection first and then assign it to FilteredExternalTools at the end. Otherwise you'll be generated a bunch of changed events (no items, then 1 item, then 2 items, etc)... I bet the titlebar could potentially flicker based on system load? #Resolved

Well, that could only happen on first init, since the user is only adding/removing one tool at a time. If we reinstate the ability to bulk edit the json, then when we read it in, it would again be an init.

Ahh... yeah, I misread that.

timkur · 2024-05-21T20:47:29Z

tools/PI/DevHome.PI/Helpers/ExternalTool.cs

-    [JsonIgnore]
+
+    [ObservableProperty]
+    private bool _isPinned;


I'm struggling with this one a bit.... first we've got the pinned property, and then potientially an "order index in the bar"... is this a property of an ExternalTool, or how it's represented in the bar? Or is it ok to have them all in one data structure.

Not advocating for a particular change now, but just something to keep in mind in the future.
#Resolved

IsPinned is rightfully a property of ExternalTool, same as arguments - both are PI-specific configurations for the tool that the user gets to choose. We use the IsPinned user setting to do X, and the Args user setting to do Y. Not sure where the "order index" comes from?

We chatted a bit about long term vision. I'm ok with this for now.

timkur · 2024-05-21T20:50:22Z

tools/PI/DevHome.PI/Helpers/ExternalToolsHelper.cs

+    }
+
+    // The bar shows only the pinned tools.
+    public ObservableCollection<ExternalTool> FilteredExternalTools { get; private set; } = [];


I think this should probably be in the BarWindow view model vs here... since the BarWindow would want a filtered view based on pinned, but other components using this (thinking about when we need input/output filters, extensions, etc) would want a different set of filtered tools. #Resolved

Yes, and that's a can of worms I want to defer to next round. ExternalToolsHelper should be split into a BarViewModel and other stuff.

zadesai · 2024-05-21T22:41:14Z

tools/PI/DevHome.PI/Helpers/ExternalTool.cs

+
+    // Note the additional "property:" syntax to ensure the JsonIgnore is propagated to the generated property.
+    [ObservableProperty]
+    [property: JsonIgnore]
    private SoftwareBitmapSource? _toolIcon;


_toolIcon

nit: can we use camelCase?

Yes that would be my preferred style, but I'm seeing the leading underscore in other parts of DevHome. We should make an explicit style choice across the board, and do a code cleanup operation at some point.

timkur

jaholme

Enable the user to unpin/pin an external tool on the bar

3c629bd

andreww-msft requested review from aeloros, jaholme, timkur and zadesai May 20, 2024 17:51

krschau reviewed May 20, 2024

View reviewed changes

Updates per feedback

a8399fb

andreww-msft self-assigned this May 21, 2024

timkur reviewed May 21, 2024

View reviewed changes

zadesai reviewed May 21, 2024

View reviewed changes

Made FilteredExternalTools a ReadOnlyObservableCollection

3adcb43

timkur approved these changes May 21, 2024

View reviewed changes

jaholme approved these changes May 22, 2024

View reviewed changes

andreww-msft merged commit 5aaa89d into main May 22, 2024
4 checks passed

andreww-msft deleted the user/andreww/pinUnpin branch May 22, 2024 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable the user to unpin/pin an external tool on the bar #2972

Enable the user to unpin/pin an external tool on the bar #2972

andreww-msft commented May 20, 2024

krschau May 20, 2024

andreww-msft May 20, 2024

krschau May 20, 2024

andreww-msft May 20, 2024

krschau May 20, 2024

andreww-msft May 20, 2024

krschau May 20, 2024

andreww-msft May 20, 2024

krschau May 20, 2024

andreww-msft May 20, 2024

timkur May 21, 2024 •

edited

Loading

andreww-msft May 21, 2024

timkur May 21, 2024 •

edited

Loading

andreww-msft May 21, 2024

timkur May 21, 2024

andreww-msft May 21, 2024

timkur May 21, 2024 •

edited

Loading

andreww-msft May 21, 2024

timkur May 21, 2024

timkur May 21, 2024 •

edited

Loading

andreww-msft May 21, 2024

timkur May 21, 2024

timkur May 21, 2024 •

edited

Loading

andreww-msft May 21, 2024

zadesai May 21, 2024

andreww-msft May 21, 2024

timkur left a comment

jaholme left a comment

	// %LocalAppData%\Packages\Microsoft.Windows.DevHome.Dev_8wekyb3d8bbwe\LocalState\externaltools.json
	// %LocalAppData%\Packages\Microsoft.Windows.DevHome_8wekyb3d8bbwe\LocalState\externaltools.json

	_log.Error(ex, "Failed to parse {tool}", toolInfoFileName);
	_log.Error(ex, $"Failed to parse {toolInfoFileName}");

Enable the user to unpin/pin an external tool on the bar #2972

Enable the user to unpin/pin an external tool on the bar #2972

Conversation

andreww-msft commented May 20, 2024

Summary of the pull request

Fixed related bugs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timkur May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timkur May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timkur May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timkur May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timkur May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timkur left a comment

Choose a reason for hiding this comment

jaholme left a comment

Choose a reason for hiding this comment

timkur May 21, 2024 •

edited

Loading

timkur May 21, 2024 •

edited

Loading

timkur May 21, 2024 •

edited

Loading

timkur May 21, 2024 •

edited

Loading

timkur May 21, 2024 •

edited

Loading