Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
May 22, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3c629bd
Enable the user to unpin/pin an external tool on the bar
andreww-msft May 20, 2024
a8399fb
Updates per feedback
andreww-msft May 20, 2024
3adcb43
Made FilteredExternalTools a ReadOnlyObservableCollection
andreww-msft May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions tools/PI/DevHome.PI/BarWindow.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -131,16 +128,16 @@

<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
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>
Expand Down
68 changes: 39 additions & 29 deletions tools/PI/DevHome.PI/BarWindow.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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; }
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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,
Expand All @@ -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))
Copy link
Contributor

@timkur timkur May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

==

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)
Expand All @@ -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));
}
}
Expand Down Expand Up @@ -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;
}
}

Expand Down
64 changes: 22 additions & 42 deletions tools/PI/DevHome.PI/Helpers/ExternalTool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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));

Expand All @@ -44,50 +45,36 @@ public string OtherArgs
{
get; private set;
}

[JsonIgnore]

[ObservableProperty]
private bool _isPinned;
Copy link
Contributor

@timkur timkur May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_toolIcon

nit: can we use camelCase?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


[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,
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();

Expand All @@ -102,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)
Expand All @@ -118,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,
Expand Down Expand Up @@ -174,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));
}
}
Loading
Loading