Skip to content
This repository was archived by the owner on Jun 5, 2025. It is now read-only.

[Environments] Adding configuration flow #3492

Merged
merged 6 commits into from
Jul 26, 2024
Merged

Conversation

huzaifa-d
Copy link
Contributor

@huzaifa-d huzaifa-d commented Jul 24, 2024

Summary of the pull request

This PR adds the feature to start the configuration flow from the Environments page.
Adds a new button, Setup, which directly takes the UI to the setup flow page.

Adding.config.flow.mp4

PR checklist

@huzaifa-d huzaifa-d force-pushed the user/modanish/AddConfigToEnv branch from 4e9c162 to f100da9 Compare July 24, 2024 20:07
@huzaifa-d huzaifa-d requested review from bbonaby and sshilov7 July 24, 2024 20:07
@huzaifa-d huzaifa-d marked this pull request as ready for review July 24, 2024 20:08
@@ -73,6 +75,7 @@ public partial class ComputeSystemViewModel : ComputeSystemCardBase, IRecipient<
IComputeSystem system,
ComputeSystemProvider provider,
Func<ComputeSystemCardBase, bool> removalAction,
Action<ComputeSystemReviewItem>? configurationAction,
Copy link
Contributor

Choose a reason for hiding this comment

The 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 (_configurationAction) on line 61 in this PR. As it'll be in the UIActions property. Your codeflow will remain the same, this just helps keep the constructor more manageable. In the future we can reduce the number of parameters further by doing more consolidation, but I think for what we're doing in this PR, this should suffice.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

@@ -151,6 +152,12 @@ public static List<OperationsViewModel> FillLaunchButtonOperations(ComputeSystem
_stringResource.GetLocalized("Operations_Terminate"), "\uEE95", computeSystem.TerminateAsync, ComputeSystemOperations.Terminate));
}

if (supportedOperations.HasFlag(ComputeSystemOperations.ApplyConfiguration) && configurationCallback is not null)
{
operations.Add(new OperationsViewModel(
Copy link
Contributor

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 code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

@@ -5,6 +5,7 @@
using System.Threading.Tasks;
using CommunityToolkit.Mvvm.Input;
using CommunityToolkit.Mvvm.Messaging;
Copy link
Contributor

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 file as I believe with those changes we won't need to make any changes to this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

@@ -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));
Copy link
Contributor

@bbonaby bbonaby Jul 24, 2024

Choose a reason for hiding this comment

The 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 ComputeSystemViewModel class like:

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 FillLaunchButtonOperations call without the need to pass the provider or the action to the DataExtractor. This way you don't need to update code in the DataExtractor or OperationsViewModel. We'd just be using the constructor for OperationsViewModel that already exists which takes in an Action.

We'll end up with something like:

RegisterForAllOperationMessages(DataExtractor.FillDotButtonOperations(ComputeSystem, _mainWindow), DataExtractor.FillLaunchButtonOperations(ComputeSystem));
AddStartSetupFlowOperation();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

@huzaifa-d huzaifa-d merged commit 48e8dd4 into main Jul 26, 2024
4 checks passed
@huzaifa-d huzaifa-d deleted the user/modanish/AddConfigToEnv branch July 26, 2024 17:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option for a user to 'configure' an environment from the environments page
6 participants