-
Notifications
You must be signed in to change notification settings - Fork 337
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)); | ||
|
||
|
@@ -44,50 +45,36 @@ public string OtherArgs | |
{ | ||
get; private set; | ||
} | ||
|
||
[JsonIgnore] | ||
|
||
[ObservableProperty] | ||
private bool _isPinned; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
||
|
@@ -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) | ||
|
@@ -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, | ||
|
@@ -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)); | ||
} | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.