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

Jump to bottom

Add a few more fixes for environments #2892

Merged
merged 3 commits into from
May 14, 2024
Merged

Add a few more fixes for environments #2892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee9b503
add small fixes
bbonaby May 12, 2024
0dddbe2
update string
bbonaby May 13, 2024
7155ed7
add comment based on PR feedback
bbonaby May 13, 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
39 changes: 34 additions & 5 deletions common/Environments/Helpers/ComputeSystemHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.IO;
using System.Runtime.InteropServices.WindowsRuntime;
using System.Threading.Tasks;
Expand All @@ -15,6 +16,8 @@ namespace DevHome.Common.Environments.Helpers;

public static class ComputeSystemHelpers
{
private static readonly ILogger _log = Log.ForContext("SourceContext", nameof(ComputeSystemHelpers));

public static async Task<byte[]?> GetBitmapImageArrayAsync(ComputeSystemCache computeSystem)
{
try
Expand All @@ -23,7 +26,7 @@ public static class ComputeSystemHelpers

if ((result.Result.Status == ProviderOperationStatus.Failure) || (result.ThumbnailInBytes.Length <= 0))
{
Log.Error($"Failed to get thumbnail for compute system {computeSystem}. Error: {result.Result.DiagnosticText}");
_log.Error($"Failed to get thumbnail for compute system {computeSystem}. Error: {result.Result.DiagnosticText}");

// No thumbnail available, return null so that the card will display the default image.
return null;
Expand All @@ -33,7 +36,7 @@ public static class ComputeSystemHelpers
}
catch (Exception ex)
{
Log.Error(ex, $"Failed to get thumbnail for compute system {computeSystem}.");
_log.Error(ex, $"Failed to get thumbnail for compute system {computeSystem}.");
return null;
}
}
Expand All @@ -48,7 +51,7 @@ public static class ComputeSystemHelpers
}
catch (Exception ex)
{
Log.Error(ex, "Failed to get thumbnail from a byte array.");
_log.Error(ex, "Failed to get thumbnail from a byte array.");
return null;
}
}
Expand All @@ -74,7 +77,7 @@ public static List<CardProperty> GetComputeSystemCardProperties(IEnumerable<Comp
}
catch (Exception ex)
{
Log.Error(ex, $"Failed to get all ComputeSystemCardProperties.");
_log.Error(ex, $"Failed to get all ComputeSystemCardProperties.");
return propertyList;
}
}
Expand All @@ -88,7 +91,7 @@ public static async Task<List<CardProperty>> GetComputeSystemCardPropertiesAsync
}
catch (Exception ex)
{
Log.Error(ex, $"Failed to get all properties for compute system {computeSystem}.");
_log.Error(ex, $"Failed to get all properties for compute system {computeSystem}.");
return new List<CardProperty>();
}
}
Expand Down Expand Up @@ -126,4 +129,30 @@ public static EnvironmentsCallToActionData UpdateCallToActionText(int providerCo

return new(navigateToExtensionsLibrary, callToActionText, callToActionHyperLinkText);
}

/// <summary>
/// Safely remove all items from an observable collection.
/// </summary>
/// <remarks>
/// There can be random COM exceptions due to using the "Clear()" method in an observable collection. This method
/// is used so that we can safely clear the observable collection without throwing an exceptions. This is related
/// to this GitHub issue https://github.com/microsoft/microsoft-ui-xaml/issues/8684. To work around this,
/// this method is used to remove all items individually from the end of the collection to the beginning of the collection.
/// </remarks>
/// <typeparam name="T">Type of objects that the collection contains</typeparam>
/// <param name="collection">An observable collection that contains zero to N elements</param>
public static void RemoveAllItems<T>(ObservableCollection<T> collection)
bbonaby marked this conversation as resolved.
Show resolved Hide resolved
{
try
{
for (var i = collection.Count - 1; i >= 0; i--)
{
collection.RemoveAt(i);
}
}
catch (Exception ex)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we get an exception, would we still want to try to clear the rest of the collection, or is it not worth 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.

I believe it's not worth it, as it may result in another exception

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to figure out the root cause of the issue. May be open a lower priority bug. Then we could understand what's the best workaround is. For example, if it's caused by a race then using a lock around Clear() could be a better solution.
One side effect of removing items one by one could be sending too many/often notifications to the subscribers, which can affect UX responsiveness or cause some issues within the subscribers if they didn't expect that.

{
_log.Error(ex, "Unable to remove items from the collection");
}
}
}
30 changes: 22 additions & 8 deletions tools/Environments/DevHome.Environments/Helpers/DataExtractor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ public static List<OperationsViewModel> FillDotButtonOperations(ComputeSystemCac

if (supportedOperations.HasFlag(ComputeSystemOperations.Restart))
{
operations.Add(new OperationsViewModel("Restart", "\uE777", computeSystem.RestartAsync, ComputeSystemOperations.Restart));
operations.Add(new OperationsViewModel(
_stringResource.GetLocalized("Operations_Restart"), "\uE777", computeSystem.RestartAsync, ComputeSystemOperations.Restart));
}

if (supportedOperations.HasFlag(ComputeSystemOperations.Delete))
Expand Down Expand Up @@ -95,37 +96,50 @@ public static List<OperationsViewModel> FillLaunchButtonOperations(ComputeSystem

if (supportedOperations.HasFlag(ComputeSystemOperations.Start))
{
operations.Add(new OperationsViewModel("Start", "\uE768", computeSystem.StartAsync, ComputeSystemOperations.Start));
operations.Add(new OperationsViewModel(
_stringResource.GetLocalized("Operations_Start"), "\uE768", computeSystem.StartAsync, ComputeSystemOperations.Start));
}

if (supportedOperations.HasFlag(ComputeSystemOperations.ShutDown))
{
operations.Add(new OperationsViewModel("Stop", "\uE71A", computeSystem.ShutDownAsync, ComputeSystemOperations.ShutDown));
operations.Add(new OperationsViewModel(
_stringResource.GetLocalized("Operations_ShutDown"), "\uE71A", computeSystem.ShutDownAsync, ComputeSystemOperations.ShutDown));
}

if (supportedOperations.HasFlag(ComputeSystemOperations.Save))
{
operations.Add(new OperationsViewModel(
_stringResource.GetLocalized("Operations_Save"), "\uE74E", computeSystem.SaveAsync, ComputeSystemOperations.Save));
}

if (supportedOperations.HasFlag(ComputeSystemOperations.CreateSnapshot))
{
operations.Add(new OperationsViewModel("Checkpoint", "\uE7C1", computeSystem.CreateSnapshotAsync, ComputeSystemOperations.CreateSnapshot));
operations.Add(new OperationsViewModel(
_stringResource.GetLocalized("Operations_CreateCheckpoint"), "\uE7C1", computeSystem.CreateSnapshotAsync, ComputeSystemOperations.CreateSnapshot));
}

if (supportedOperations.HasFlag(ComputeSystemOperations.RevertSnapshot))
{
operations.Add(new OperationsViewModel("Revert", "\uE7A7", computeSystem.RevertSnapshotAsync, ComputeSystemOperations.RevertSnapshot));
operations.Add(new OperationsViewModel(
_stringResource.GetLocalized("Operations_RevertCheckpoint"), "\uE7A7", computeSystem.RevertSnapshotAsync, ComputeSystemOperations.RevertSnapshot));
}

if (supportedOperations.HasFlag(ComputeSystemOperations.Pause))
{
operations.Add(new OperationsViewModel("Pause", "\uE769", computeSystem.PauseAsync, ComputeSystemOperations.Pause));
operations.Add(new OperationsViewModel(
_stringResource.GetLocalized("Operations_Pause"), "\uE769", computeSystem.PauseAsync, ComputeSystemOperations.Pause));
}

if (supportedOperations.HasFlag(ComputeSystemOperations.Resume))
{
operations.Add(new OperationsViewModel("Resume", "\uE768", computeSystem.ResumeAsync, ComputeSystemOperations.Resume));
operations.Add(new OperationsViewModel(
_stringResource.GetLocalized("Operations_Resume"), "\uE768", computeSystem.ResumeAsync, ComputeSystemOperations.Resume));
}

if (supportedOperations.HasFlag(ComputeSystemOperations.Terminate))
{
operations.Add(new OperationsViewModel("Terminate", "\uEE95", computeSystem.TerminateAsync, ComputeSystemOperations.Terminate));
operations.Add(new OperationsViewModel(
_stringResource.GetLocalized("Operations_Terminate"), "\uEE95", computeSystem.TerminateAsync, ComputeSystemOperations.Terminate));
}

return operations;
Expand Down
16 changes: 10 additions & 6 deletions tools/Environments/DevHome.Environments/Strings/en-us/Resources.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@
<value>Do you want to delete your environment?</value>
<comment>Text for the title of the delete environment confirmation popup.</comment>
</data>
<data name="Operations_Checkpoint" xml:space="preserve">
<data name="Operations_CreateCheckpoint" xml:space="preserve">
<value>Checkpoint</value>
<comment>Text for the creating the checkpoint operation shown for the environment.</comment>
</data>
Expand All @@ -309,20 +309,24 @@
<value>Resume</value>
<comment>Text for the resume operation shown for the environment.</comment>
</data>
<data name="Operations_Revert" xml:space="preserve">
<data name="Operations_RevertCheckpoint" xml:space="preserve">
<value>Revert</value>
<comment>Text for the revert operation shown for the environment.</comment>
</data>
<data name="Operations_Start" xml:space="preserve">
<value>Start</value>
<comment>Text for the start operation shown for the environment.</comment>
</data>
<data name="Operations_Stop" xml:space="preserve">
<value>Stop</value>
<comment>Text for the stop operation shown for the environment.</comment>
<data name="Operations_Shutdown" xml:space="preserve">
<value>Shutdown</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't "Stop" the value that the designers wanted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually they decided to change it #2665 to match hyper-v

Copy link
Contributor

Choose a reason for hiding this comment

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

So don't you want to link and close the issue with this PR?

<comment>Text for the shutdown operation shown for the environment.</comment>
</data>
<data name="Operations_Terminate" xml:space="preserve">
<value>Terminate</value>
<value>Turn off</value>
<comment>Text for the terminate operation shown for the environment.</comment>
</data>
<data name="Operations_Save" xml:space="preserve">
<value>Save</value>
<comment>Text for the terminate operation shown for the environment.</comment>
</data>
<data name="LaunchingEnvironmentText" xml:space="preserve">
Expand Down
12 changes: 12 additions & 0 deletions tools/Environments/DevHome.Environments/ViewModels/ComputeSystemCardBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using DevHome.Common.Environments.Models;
using Microsoft.UI.Xaml.Media.Imaging;
using Microsoft.Windows.DevHome.SDK;
using Windows.Foundation;

namespace DevHome.Environments.ViewModels;

Expand All @@ -23,6 +24,8 @@ public abstract partial class ComputeSystemCardBase : ObservableObject

public bool IsCreateComputeSystemOperation { get; protected set; }

public event TypedEventHandler<ComputeSystemCardBase, string>? ComputeSystemErrorReceived;

// Will hold the supported actions that the user can perform on in the UI. E.g Remove button
public ObservableCollection<OperationsViewModel> DotOperations { get; protected set; } = new();

Expand Down Expand Up @@ -55,4 +58,13 @@ public override string ToString()
{
return $"{Name} {AlternativeName}";
}

/// <summary>
/// Common way to send error message to UI for classes that implement ComputeSystemCardBase
/// </summary>
/// <param name="errorMessage">Error message to send to the user</param>
public virtual void OnErrorReceived(string errorMessage)
{
ComputeSystemErrorReceived?.Invoke(this, errorMessage);
}
}
9 changes: 4 additions & 5 deletions tools/Environments/DevHome.Environments/ViewModels/ComputeSystemViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@ public partial class ComputeSystemViewModel : ComputeSystemCardBase, IRecipient<

public ComputeSystemCache ComputeSystem { get; protected set; }

public event TypedEventHandler<ComputeSystemViewModel, string>? ComputeSystemErrorFound;

// Launch button operations
public ObservableCollection<OperationsViewModel> LaunchOperations { get; set; } = new();

Expand Down Expand Up @@ -165,7 +163,7 @@ private async void SetPropertiesAsync()
var properties = await ComputeSystemHelpers.GetComputeSystemCardPropertiesAsync(ComputeSystem!, PackageFullName);
lock (_lock)
{
Properties.Clear();
ComputeSystemHelpers.RemoveAllItems(Properties);
foreach (var property in properties)
{
Properties.Add(property);
Expand All @@ -177,7 +175,8 @@ public void OnComputeSystemStateChanged(ComputeSystem sender, ComputeSystemState
{
_windowEx.DispatcherQueue.EnqueueAsync(async () =>
{
if (sender.Id == ComputeSystem.Id.Value)
if (sender.Id == ComputeSystem.Id.Value &&
sender.AssociatedProviderId.Equals(ComputeSystem.AssociatedProviderId.Value, StringComparison.OrdinalIgnoreCase))
{
State = newState;
StateColor = ComputeSystemHelpers.GetColorBasedOnState(newState);
Expand Down Expand Up @@ -269,7 +268,7 @@ private void LogFailure(ComputeSystemOperationResult? operationResult)
}

// Show the error notification to tell the user the operation failed
ComputeSystemErrorFound?.Invoke(this, errorMessage);
OnErrorReceived(errorMessage);
}

/// <summary>
Expand Down
4 changes: 3 additions & 1 deletion tools/Environments/DevHome.Environments/ViewModels/CreateComputeSystemOperationViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,9 @@ private void UpdateStatusIfCompleted(CreateComputeSystemResult createComputeSyst
}
else
{
UpdateUiMessage(_stringResource.GetLocalized("FailureMessageForCreateComputeSystem", createComputeSystemResult.Result.DisplayMessage));
// Reset text in UI card and show the error notification info bar to tell the user the operation failed
UpdateUiMessage(string.Empty);
OnErrorReceived(_stringResource.GetLocalized("FailureMessageForCreateComputeSystem", createComputeSystemResult.Result.DisplayMessage));
State = ComputeSystemState.Unknown;
StateColor = CardStateColor.Failure;
}
Expand Down
16 changes: 11 additions & 5 deletions tools/Environments/DevHome.Environments/ViewModels/LandingPageViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ public async Task LoadModelAsync(bool useDebugValues = false)
if (ComputeSystemCards[i] is ComputeSystemViewModel computeSystemViewModel)
{
computeSystemViewModel.RemoveStateChangedHandler();
computeSystemViewModel.ComputeSystemErrorFound -= OnComputeSystemOperationError;
ComputeSystemCards[i].ComputeSystemErrorReceived -= OnComputeSystemOperationError;
ComputeSystemCards.RemoveAt(i);
}
}
Expand Down Expand Up @@ -273,6 +273,7 @@ private void SetupCreateComputeSystemOperationForUI()
{
var operationViewModel = ComputeSystemCards[i] as CreateComputeSystemOperationViewModel;
operationViewModel!.RemoveEventHandlers();
operationViewModel.ComputeSystemErrorReceived -= OnComputeSystemOperationError;
ComputeSystemCards.RemoveAt(i);
}
}
Expand All @@ -282,8 +283,11 @@ private void SetupCreateComputeSystemOperationForUI()
{
// this is a new operation so we need to create a view model for it.
ComputeSystemCards.Add(new CreateComputeSystemOperationViewModel(_computeSystemManager, _stringResource, _windowEx, RemoveComputeSystemCard, AddNewlyCreatedComputeSystem, operation));
ComputeSystemCards.Last().ComputeSystemErrorReceived += OnComputeSystemOperationError;
_log.Information($"Found new create compute system operation for provider {operation.ProviderDetails.ComputeSystemProvider}, with name {operation.EnvironmentName}");
}

UpdateCallToActionText();
}
}

Expand Down Expand Up @@ -314,7 +318,7 @@ private async Task AddAllComputeSystemsFromAProvider(ComputeSystemsLoadedData da
packageFullName,
_windowEx);

computeSystemViewModel.ComputeSystemErrorFound += OnComputeSystemOperationError;
computeSystemViewModel.ComputeSystemErrorReceived += OnComputeSystemOperationError;
computeSystemViewModels.Add(computeSystemViewModel);
}

Expand Down Expand Up @@ -451,6 +455,7 @@ private void AddNewlyCreatedComputeSystem(ComputeSystemViewModel computeSystemVi
{
lock (ComputeSystemCards)
{
computeSystemViewModel.ComputeSystemErrorReceived += OnComputeSystemOperationError;
ComputeSystemCards.Add(computeSystemViewModel);
}
});
Expand All @@ -466,11 +471,11 @@ private bool RemoveComputeSystemCard(ComputeSystemCardBase computeSystemCard)
}
}

private void OnComputeSystemOperationError(ComputeSystemViewModel computeSystemViewModel, string errorText)
private void OnComputeSystemOperationError(ComputeSystemCardBase cardBase, string errorText)
{
_notificationsHelper?.DisplayComputeSystemOperationError(
computeSystemViewModel.ProviderDisplayName,
computeSystemViewModel.ComputeSystem!.DisplayName.Value,
cardBase.ProviderDisplayName,
cardBase.Name,
errorText);
}

Expand Down Expand Up @@ -499,6 +504,7 @@ private void UpdateCallToActionText()
// if there are cards in the UI don't update the text and keep their values as null.
if (ComputeSystemCards.Count > 0)
{
CallToActionText = null;
return;
}

Expand Down
5 changes: 3 additions & 2 deletions tools/SetupFlow/DevHome.SetupFlow/ViewModels/Environments/ComputeSystemCardViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ public void OnComputeSystemStateChanged(ComputeSystem sender, ComputeSystemState
{
_windowEx.DispatcherQueue.EnqueueAsync(async () =>
{
if (sender.Id == ComputeSystem.Id.Value)
if (sender.Id == ComputeSystem.Id.Value &&
sender.AssociatedProviderId.Equals(ComputeSystem.AssociatedProviderId.Value, StringComparison.OrdinalIgnoreCase))
{
CardState = state;
StateColor = ComputeSystemHelpers.GetColorBasedOnState(state);
Expand All @@ -101,7 +102,7 @@ private async Task UpdatePropertiesAsync()
var properties = await ComputeSystemHelpers.GetComputeSystemCardPropertiesAsync(ComputeSystem, _packageFullName);
lock (_lock)
{
ComputeSystemProperties.Clear();
ComputeSystemHelpers.RemoveAllItems(ComputeSystemProperties);
foreach (var property in properties)
{
ComputeSystemProperties.Add(property);
Expand Down
Loading
Loading