-
Notifications
You must be signed in to change notification settings - Fork 2
[Environments] Adding configuration flow #3492
Changes from 4 commits
f77c2ab
990338f
026fe98
f100da9
8ff176c
b084f14
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 |
---|---|---|
|
@@ -58,6 +58,8 @@ public partial class ComputeSystemViewModel : ComputeSystemCardBase, IRecipient< | |
|
||
private bool _disposedValue; | ||
|
||
private Action<ComputeSystemReviewItem>? _configurationAction; | ||
|
||
/// <summary> | ||
/// Initializes a new instance of the <see cref="ComputeSystemViewModel"/> class. | ||
/// This class requires a 3-step initialization: | ||
|
@@ -73,6 +75,7 @@ public ComputeSystemViewModel( | |
IComputeSystem system, | ||
ComputeSystemProvider provider, | ||
Func<ComputeSystemCardBase, bool> removalAction, | ||
Action<ComputeSystemReviewItem>? configurationAction, | ||
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. Since this constructor is getting pretty big, we should combine some of these parameters into objects. E.g we can have an object to contain the actions like: // In a new class called DevHomeUIActions
public Func<ComputeSystemCardBase, bool> RemoveComputeSystem;
public Action<ComputeSystemReviewItem>? StartComputeSystemSetup;
public DevHomeUIActions(
Func<ComputeSystemCardBase, bool> removalAction,
Action<ComputeSystemReviewItem>? startConfigurationFlowAction)
{
// add to public properties....
RemoveComputeSystem = removalAction;
StartComputeSystemSetup = startConfigurationFlowAction
}
///// DevHomeUIActions end
// Then use it as a parameter for the `ComputeSystemViewModel`'s constructor
private readonly ComputeSystemReviewItem _reviewItem;
private readonly DevHomeUIActions _uiActions;
public ComputeSystemViewModel(
IComputeSystemManager manager,
IComputeSystem system,
ComputeSystemProvider provider,
DevHomeUIActions uiActions,
string packageFullName,
Window window)
{
...
ComputeSystem = new(system);
_uiActions = uiActions;
_reviewItem = new (ComputeSystem , provider);
...
} If we do it this way, we don't need to add the new field above ( Since we know the provider and the ComputeSystemCache are available in the constructor, we can also create the ComputeSystemReviewItem directly in the constructor. See comment below on how we can use it 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. Gonna skip this for a later PR that might add more; quite a few examples with 7 or more parameters in the constructor. 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. Adding my 2-cents. A ctor with many parameters tells me that this class is trying to do too many things. I wonder if this class can be split? No need to respond to this comment. |
||
string packageFullName, | ||
Window window) | ||
{ | ||
|
@@ -83,6 +86,7 @@ public ComputeSystemViewModel( | |
ComputeSystem = new(system); | ||
PackageFullName = packageFullName; | ||
_removalAction = removalAction; | ||
_configurationAction = configurationAction; | ||
_stringResource = new StringResource("DevHome.Environments.pri", "DevHome.Environments/Resources"); | ||
} | ||
|
||
|
@@ -133,7 +137,7 @@ private async Task InitializeOperationDataAsync() | |
ShouldShowDotOperations = false; | ||
ShouldShowSplitButton = false; | ||
|
||
RegisterForAllOperationMessages(DataExtractor.FillDotButtonOperations(ComputeSystem, _mainWindow), DataExtractor.FillLaunchButtonOperations(ComputeSystem)); | ||
RegisterForAllOperationMessages(DataExtractor.FillDotButtonOperations(ComputeSystem, _mainWindow), DataExtractor.FillLaunchButtonOperations(_provider, ComputeSystem, _configurationAction)); | ||
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. With the above changes we can then simplify what you did in the DataExtractor. We can have a method in this private void AddStartSetupFlowOperation()
{
var operations = ComputeSystem.SupportedOperations.Value;
if (!operations .HasFlag(ComputeSystemOperations.ApplyConfiguration) ||
_uiActions.StartComputeSystemSetup == null))
{
return;
}
// Use lambda to create Action that does not take a parameter
var configurationAction = () =>
{
_uiActions.StartComputeSystemSetup(_reviewItem)
};
LaunchOperations.Add(new OperationsViewModel(
_stringResource.GetLocalized("Operations_ApplyConfiguration"), "\uE835", configurationAction));
} This can be called right after this We'll end up with something like: RegisterForAllOperationMessages(DataExtractor.FillDotButtonOperations(ComputeSystem, _mainWindow), DataExtractor.FillLaunchButtonOperations(ComputeSystem));
AddStartSetupFlowOperation(); 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. Same as above. |
||
|
||
_ = Task.Run(async () => | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
using System.Threading.Tasks; | ||
using CommunityToolkit.Mvvm.Input; | ||
using CommunityToolkit.Mvvm.Messaging; | ||
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. See comments in 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. Same as above. |
||
using DevHome.Common.Environments.Models; | ||
using DevHome.Common.Services; | ||
using DevHome.Environments.Models; | ||
using Microsoft.UI.Xaml; | ||
|
@@ -37,6 +38,10 @@ public partial class OperationsViewModel : IEquatable<OperationsViewModel> | |
|
||
private readonly string _additionalContext = string.Empty; | ||
|
||
private readonly Window? _mainWindow; | ||
|
||
private readonly StringResource _stringResource = new("DevHome.Environments.pri", "DevHome.Environments/Resources"); | ||
|
||
public string Name { get; } | ||
|
||
public ComputeSystemOperations ComputeSystemOperation { get; } | ||
|
@@ -47,9 +52,9 @@ public partial class OperationsViewModel : IEquatable<OperationsViewModel> | |
|
||
private Action? DevHomeAction { get; } | ||
|
||
private readonly Window? _mainWindow; | ||
private Action<ComputeSystemReviewItem>? DevHomeActionWithReviewItem { get; } | ||
|
||
private readonly StringResource _stringResource = new("DevHome.Environments.pri", "DevHome.Environments/Resources"); | ||
private ComputeSystemReviewItem? _item; | ||
|
||
public OperationsViewModel( | ||
string name, | ||
|
@@ -89,14 +94,36 @@ public OperationsViewModel(string name, string icon, Action command) | |
DevHomeAction = command; | ||
} | ||
|
||
public OperationsViewModel( | ||
string name, | ||
string icon, | ||
Action<ComputeSystemReviewItem>? command, | ||
ComputeSystemProvider provider, | ||
ComputeSystemCache cache) | ||
{ | ||
_operationKind = OperationKind.DevHomeAction; | ||
Name = name; | ||
IconGlyph = icon; | ||
DevHomeActionWithReviewItem = command; | ||
_item = new(cache, provider); | ||
} | ||
|
||
private void RunAction() | ||
{ | ||
// To Do: Need to disable the card UI while the operation is in progress and handle failures. | ||
Task.Run(async () => | ||
{ | ||
if (_operationKind == OperationKind.DevHomeAction) | ||
{ | ||
DevHomeAction!(); | ||
if (DevHomeAction != null) | ||
{ | ||
DevHomeAction(); | ||
} | ||
else if (DevHomeActionWithReviewItem != null && _item != null) | ||
{ | ||
DevHomeActionWithReviewItem(_item); | ||
} | ||
|
||
return; | ||
} | ||
|
||
|
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.
see comments in
ComputeSystemViewModel.cs
, I don't think we'll need to update the DataExtractor codeThere 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.
Same as above.