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.

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

More caches added, other improvements #2551

Merged
merged 9 commits into from
Apr 10, 2024
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public static IServiceCollection AddWindowsCustomization(this IServiceCollection
services.AddSingleton<DeveloperFileExplorerViewModel>();
services.AddTransient<DeveloperFileExplorerPage>();

services.AddSingleton<OptimizeDevDriveDialogViewModelFactory>(sp => (cacheLocation, environmentVariable) => ActivatorUtilities.CreateInstance<OptimizeDevDriveDialogViewModel>(sp, cacheLocation, environmentVariable));
services.AddSingleton<OptimizeDevDriveDialogViewModelFactory>(sp => (cacheLocation, environmentVariable, exampleDevDriveLocation, existingDevDriveLetters) => ActivatorUtilities.CreateInstance<OptimizeDevDriveDialogViewModel>(sp, cacheLocation, environmentVariable, exampleDevDriveLocation, existingDevDriveLetters));
Copy link
Contributor

@dhoehna dhoehna Apr 9, 2024

Choose a reason for hiding this comment

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

What is this doing? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was needed to pass parameters to the Dialog and from the caller UX and remove the DependencyProperties.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry. Let me a bit more clear. I can't parse what this code is doing. I've never seen => being used twice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We also define a ViewModel factory this way in the Dashboard, I think I got the syntax from somewhere in SetupFlow. According to a robot,

The first part sp => indicates the input parameter of the lambda expression. In this case, sp represents the IServiceProvider instance that will be passed to the lambda expression.

The second part (cacheLocation, environmentVariable, exampleDevDriveLocation, existingDevDriveLetters) => represents the body of the lambda expression. It defines the logic that will be executed when the lambda expression is invoked.

The body of the lambda expression is ActivatorUtilities.CreateInstance<OptimizeDevDriveDialogViewModel>(sp, cacheLocation, environmentVariable, exampleDevDriveLocation, existingDevDriveLetters). It calls the ActivatorUtilities.CreateInstance method to create an instance of the OptimizeDevDriveDialogViewModel class, passing in the sp (service provider) and the three parameters.

Overall, the lambda expression defines a delegate that can be invoked to create an instance of the OptimizeDevDriveDialogViewModel class with the specified parameters, using the ActivatorUtilities.CreateInstance method.

Clear as mud :)

Copy link
Contributor

Choose a reason for hiding this comment

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

oh
two lambda's. Okay. I understand more.

Maybe add some comments, or expand sp to serviceProvider or use some whitespace to make this layout more clear.

services.AddSingleton<DevDriveInsightsViewModel>();
services.AddTransient<DevDriveInsightsPage>();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@ public partial class DevDriveCacheData

public List<string>? CacheDirectory { get; set; }

public string? ExampleDirectory { get; set; }
public string? ExampleSubDirectory { get; set; }
}
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@
<comment>Dev drive size free</comment>
</data>
<data name="DevDriveInsightsCard.Description" xml:space="preserve">
<value>All things, dev drives, optimizations, etc.</value>
<value>All things, Dev Drives, optimizations, etc.</value>
<comment>The description for the Dev Drive Insights settings card</comment>
</data>
<data name="DevDriveInsightsCard.Header" xml:space="preserve">
Expand Down Expand Up @@ -161,9 +161,9 @@
<value>Enable end task in taskbar by right click</value>
<comment>The description for the end task on task bar settings card</comment>
</data>
<data name="ExampleDevDriveLocation" xml:space="preserve">
<value>Example: E:\packages\pip</value>
<comment>Example dev drive location</comment>
<data name="ExampleText" xml:space="preserve">
<value>Example: </value>
Copy link
Collaborator

@krschau krschau Apr 9, 2024

Choose a reason for hiding this comment

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

Don't include space here, I'm not sure it gets translated. #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is getting reflected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was referring to the files that aren't checked in, the ones translated to other languages. But it's probably fine.

<comment>Example string, will be followed by a sample location to move the cache to a dev drive location</comment>
</data>
<data name="EndTaskOnTaskBar.Header" xml:space="preserve">
<value>End Task</value>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using CommunityToolkit.Mvvm.ComponentModel;
using CommunityToolkit.Mvvm.Input;
Expand All @@ -18,6 +19,8 @@ public partial class DevDriveOptimizerCardViewModel : ObservableObject
{
public OptimizeDevDriveDialogViewModelFactory OptimizeDevDriveDialogViewModelFactory { get; set; }

public List<string> ExistingDevDriveLetters { get; set; }

public string CacheToBeMoved { get; set; }

public string DevDriveOptimizationSuggestion { get; set; }
Expand All @@ -41,17 +44,28 @@ private async Task OptimizeDevDriveAsync(object sender)
var settingsCard = sender as Button;
if (settingsCard != null)
{
var optimizeDevDriveViewModel = OptimizeDevDriveDialogViewModelFactory(ExistingCacheLocation, EnvironmentVariableToBeSet);
var optimizeDevDriveViewModel = OptimizeDevDriveDialogViewModelFactory(
ExistingCacheLocation,
EnvironmentVariableToBeSet,
ExampleLocationOnDevDrive,
ExistingDevDriveLetters);
var optimizeDevDriveDialog = new OptimizeDevDriveDialog(optimizeDevDriveViewModel);
optimizeDevDriveDialog.XamlRoot = settingsCard.XamlRoot;
optimizeDevDriveDialog.RequestedTheme = settingsCard.ActualTheme;
await optimizeDevDriveDialog.ShowAsync();
}
}

public DevDriveOptimizerCardViewModel(OptimizeDevDriveDialogViewModelFactory optimizeDevDriveDialogViewModelFactory, string cacheToBeMoved, string existingCacheLocation, string exampleLocationOnDevDrive, string environmentVariableToBeSet)
public DevDriveOptimizerCardViewModel(
OptimizeDevDriveDialogViewModelFactory optimizeDevDriveDialogViewModelFactory,
string cacheToBeMoved,
string existingCacheLocation,
string exampleLocationOnDevDrive,
string environmentVariableToBeSet,
List<string> existingDevDriveLetters)
{
OptimizeDevDriveDialogViewModelFactory = optimizeDevDriveDialogViewModelFactory;
ExistingDevDriveLetters = existingDevDriveLetters;
CacheToBeMoved = cacheToBeMoved;
ExistingCacheLocation = existingCacheLocation;
ExampleLocationOnDevDrive = exampleLocationOnDevDrive;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,22 @@
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Data.SqlTypes;
using System.IO;
using System.Threading.Tasks;
using CommunityToolkit.Mvvm.ComponentModel;
using CommunityToolkit.Mvvm.Input;
using DevHome.Common.Extensions;
using DevHome.Common.Services;
using DevHome.Common.TelemetryEvents;
using DevHome.Telemetry;
using Microsoft.UI.Xaml.Controls;
using Serilog;
using Windows.Media.Protection;
using Windows.Storage.Pickers;
using WinUIEx;
using static Microsoft.Extensions.Logging.EventSource.LoggingEventSource;

namespace DevHome.Customization.ViewModels.DevDriveInsights;

Expand All @@ -19,6 +26,9 @@ namespace DevHome.Customization.ViewModels.DevDriveInsights;
/// </summary>
public partial class OptimizeDevDriveDialogViewModel : ObservableObject
{
[ObservableProperty]
private List<string> _existingDevDriveLetters;

[ObservableProperty]
private string _exampleDevDriveLocation;

Expand All @@ -40,11 +50,16 @@ public partial class OptimizeDevDriveDialogViewModel : ObservableObject
[ObservableProperty]
private string _directoryPathTextBox;

public OptimizeDevDriveDialogViewModel(string existingCacheLocation, string environmentVariableToBeSet)
public OptimizeDevDriveDialogViewModel(
string existingCacheLocation,
string environmentVariableToBeSet,
string exampleDevDriveLocation,
List<string> existingDevDriveLetters)
{
DirectoryPathTextBox = string.Empty;
var stringResource = new StringResource("DevHome.Customization.pri", "DevHome.Customization/Resources");
ExampleDevDriveLocation = stringResource.GetLocalized("ExampleDevDriveLocation");
ExistingDevDriveLetters = existingDevDriveLetters;
ExampleDevDriveLocation = stringResource.GetLocalized("ExampleText") + exampleDevDriveLocation;
ChooseDirectoryPromptText = stringResource.GetLocalized("ChooseDirectoryPromptText");
MakeChangesText = stringResource.GetLocalized("MakeChangesText");
ExistingCacheLocation = existingCacheLocation;
Expand Down Expand Up @@ -78,7 +93,20 @@ private async Task BrowseButtonClick(object sender)
}
}

private void MoveDirectory(string sourceDirectory, string targetDirectory)
private string RemovePrivacyInfo(string input)
{
var output = input;
var userProfilePath = Environment.ExpandEnvironmentVariables("%userprofile%");
if (input.StartsWith(userProfilePath, StringComparison.OrdinalIgnoreCase))
{
var index = input.LastIndexOf(userProfilePath, StringComparison.OrdinalIgnoreCase) + userProfilePath.Length;
output = Path.Join("%userprofile%", input.Substring(index));
}

return output;
}

private bool MoveDirectory(string sourceDirectory, string targetDirectory)
Copy link
Contributor

@dhoehna dhoehna Apr 10, 2024

Choose a reason for hiding this comment

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

I'm on the fence about returning bool here. Line 172 does not do anything different if the move failed. In this context the caller can assume the move finished successfully if MoveDirectory didn't throw.

On the other, maybe other parts of the code branch depending on the result.

I'll leave it up to you. C# is a "If it does not throw, we're good" #WontFix

{
try
{
Expand Down Expand Up @@ -110,10 +138,13 @@ private void MoveDirectory(string sourceDirectory, string targetDirectory)

// Delete the source directory
Directory.Delete(sourceDirectory, true);
return true;
}
catch (Exception ex)
{
Log.Error(ex, $"Error in MoveDirectory. Error: {ex}");
Log.Error($"Error in MoveDirectory. Error: {ex}");
TelemetryFactory.Get<ITelemetry>().LogError("DevDriveInsights_PackageCacheMoveDirectory_Error", LogLevel.Critical, new ExceptionEvent(ex.HResult, RemovePrivacyInfo(sourceDirectory)));
return false;
}
}

Expand All @@ -129,6 +160,19 @@ private void SetEnvironmentVariable(string variableName, string value)
}
}

private bool ChosenDirectoryInDevDrive(string directoryPath)
{
foreach (var devDriveLetter in ExistingDevDriveLetters)
{
if (directoryPath.StartsWith(devDriveLetter + ":", StringComparison.OrdinalIgnoreCase))
{
return true;
}
}

return false;
}

[RelayCommand]
private void DirectoryInputConfirmed()
{
Expand All @@ -137,9 +181,21 @@ private void DirectoryInputConfirmed()
if (!string.IsNullOrEmpty(directoryPath))
{
// Handle the selected folder
// TODO: If chosen folder not a dev drive location, currently we no-op. Instead we should display the error.
MoveDirectory(ExistingCacheLocation, directoryPath);
SetEnvironmentVariable(EnvironmentVariableToBeSet, directoryPath);
// TODO: If chosen folder not a dev drive location, currently we no-op and log the error. Instead we should display the error.
Copy link
Contributor Author

@SohamDas2021 SohamDas2021 Apr 8, 2024

Choose a reason for hiding this comment

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

// TODO: If chosen folder not a dev drive location, currently we no-op and log the error. Instead we should display the error.

Filed #2563
@bbonaby #Closed

if (ChosenDirectoryInDevDrive(directoryPath))
{
if (MoveDirectory(ExistingCacheLocation, directoryPath))
{
SetEnvironmentVariable(EnvironmentVariableToBeSet, directoryPath);
var existingCacheLocationVetted = RemovePrivacyInfo(ExistingCacheLocation);
Log.Debug($"Moved cache from {existingCacheLocationVetted} to {directoryPath}");
TelemetryFactory.Get<ITelemetry>().Log("DevDriveInsights_PackageCacheMovedSuccessfully_Event", LogLevel.Critical, new ExceptionEvent(0, existingCacheLocationVetted));
}
}
else
{
Log.Error($"Chosen directory {directoryPath} not on a dev drive.");
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,17 @@
using DevHome.Customization.Helpers;
using DevHome.Customization.ViewModels.DevDriveInsights;
using DevHome.Customization.Views;
using Microsoft.Internal.Windows.DevHome.Helpers;
using Serilog;

namespace DevHome.Customization.ViewModels;

public partial class DevDriveInsightsViewModel : ObservableObject
{
private readonly ShellSettings _shellSettings;

public ObservableCollection<Breadcrumb> Breadcrumbs { get; }

public ObservableCollection<DevDriveCardViewModel> DevDriveCardCollection { get; private set; } = new();

public ObservableCollection<DevDriveOptimizerCardViewModel> DevDriveOptimizerCardCollection { get; private set; } = new();
Expand Down Expand Up @@ -48,12 +53,29 @@ public partial class DevDriveInsightsViewModel : ObservableObject

private IEnumerable<IDevDrive> ExistingDevDrives { get; set; } = Enumerable.Empty<IDevDrive>();

private static readonly string _appDataPath = Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData);
Copy link
Contributor

Choose a reason for hiding this comment

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

@krschau I know StyleCop changed recently. I remember StyleCop wanting all readonly variables first. Did that change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still seeing this error if I don't follow the rule locally in the Dashboard project. All projects should use the same stylecop.json and globalsuppressions.cs. The static keyword seems to be making the error go away? Nothing in the doc for this rule explains what's happening.

https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1214.md


private static readonly string _localAppDataPath = Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData);

private static readonly string _userProfilePath = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile);

private const string PackagesStr = "packages";

private const string CacheStr = "cache";

private const string ArchivesStr = "archives";

public DevDriveInsightsViewModel(IDevDriveManager devDriveManager, OptimizeDevDriveDialogViewModelFactory optimizeDevDriveDialogViewModelFactory)
{
_shellSettings = new ShellSettings();

var stringResource = new StringResource("DevHome.Customization.pri", "DevHome.Customization/Resources");
Breadcrumbs =
[
new(stringResource.GetLocalized("MainPage_Header"), typeof(MainPageViewModel).FullName!),
new(stringResource.GetLocalized("DevDriveInsights_Header"), typeof(DevDriveInsightsViewModel).FullName!)
];

_optimizeDevDriveDialogViewModelFactory = optimizeDevDriveDialogViewModelFactory;
DevDriveManagerObj = devDriveManager;
}
Expand Down Expand Up @@ -237,17 +259,60 @@ public void UpdateListViewModelList()
EnvironmentVariable = "PIP_CACHE_DIR",
CacheDirectory = new List<string>
{
Path.Join(_localAppDataPath, "pip", "cache"),
Path.Join(_localAppDataPath, "packages", "PythonSoftwareFoundation.Python"),
Path.Join(_localAppDataPath, "pip", CacheStr),
Path.Join(_localAppDataPath, PackagesStr, "PythonSoftwareFoundation.Python"),
},
ExampleDirectory = Path.Join("D:", "packages", "pip", "cache"),
ExampleSubDirectory = Path.Join(PackagesStr, "pip", CacheStr),
},
new DevDriveCacheData
{
CacheName = "NuGet cache (dotnet)",
EnvironmentVariable = "NUGET_PACKAGES",
CacheDirectory = new List<string> { Path.Join(_userProfilePath, ".nuget", "packages") },
ExampleDirectory = Path.Join("D:", "packages", "NuGet", "Cache"),
CacheDirectory = new List<string> { Path.Join(_userProfilePath, ".nuget", PackagesStr) },
dhoehna marked this conversation as resolved.
Show resolved Hide resolved
ExampleSubDirectory = Path.Join(PackagesStr, "NuGet", CacheStr),
},
new DevDriveCacheData
{
CacheName = "Npm cache (NodeJS)",
Copy link
Contributor

@dhoehna dhoehna Apr 9, 2024

Choose a reason for hiding this comment

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

Also, why are these values hard coded? Shouldn't we not be doing this so DevHome can stay generic? #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently for V1, we are only looking to optimize for these caches- https://learn.microsoft.com/en-us/windows/dev-drive/#storing-package-cache-on-dev-drive.

In future we are looking to extend this to allow the user to input the existing cache location and env variable so that it works for any package cache. But that design is not finalized yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need localization?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need localization?

I don't think so because these are product names.

EnvironmentVariable = "NPM_CONFIG_CACHE",
CacheDirectory = new List<string>
{
Path.Join(_appDataPath, "npm-cache"),
Path.Join(_localAppDataPath, "npm-cache"),
},
ExampleSubDirectory = Path.Join(PackagesStr, "npm"),
},
new DevDriveCacheData
{
CacheName = "Vcpkg cache",
EnvironmentVariable = "VCPKG_DEFAULT_BINARY_CACHE",
CacheDirectory = new List<string>
{
Path.Join(_appDataPath, "vcpkg", ArchivesStr),
Path.Join(_localAppDataPath, "vcpkg", ArchivesStr),
},
ExampleSubDirectory = Path.Join(PackagesStr, "vcpkg"),
},
new DevDriveCacheData
{
CacheName = "Cargo cache (Rust)",
EnvironmentVariable = "CARGO_HOME",
CacheDirectory = new List<string> { Path.Join(_userProfilePath, ".cargo") },
ExampleSubDirectory = Path.Join(PackagesStr, "cargo"),
},
new DevDriveCacheData
{
CacheName = "Maven cache (Java)",
EnvironmentVariable = "MAVEN_OPTS",
CacheDirectory = new List<string> { Path.Join(_userProfilePath, ".m2") },
ExampleSubDirectory = Path.Join(PackagesStr, "m2"),
},
new DevDriveCacheData
{
CacheName = "Gradle cache (Java)",
EnvironmentVariable = "GRADLE_USER_HOME",
CacheDirectory = new List<string> { Path.Join(_userProfilePath, ".gradle") },
ExampleSubDirectory = Path.Join(PackagesStr, "gradle"),
}
];

Expand All @@ -261,13 +326,13 @@ public void UpdateListViewModelList()
}
else
{
var subDirectories = Directory.GetDirectories(_localAppDataPath + "\\Packages", "*", SearchOption.TopDirectoryOnly);
var subDirectories = Directory.GetDirectories(Path.Join(_localAppDataPath, PackagesStr), "*", SearchOption.TopDirectoryOnly);
var matchingSubdirectory = subDirectories.FirstOrDefault(subdir => subdir.StartsWith(cacheDirectory, StringComparison.OrdinalIgnoreCase));
if (Directory.Exists(matchingSubdirectory))
{
if (matchingSubdirectory.Contains("PythonSoftwareFoundation"))
{
return Path.Join(matchingSubdirectory, "LocalCache", "Local", "pip", "cache");
return Path.Join(matchingSubdirectory, "LocalCache", "Local", "pip", CacheStr);
}

return matchingSubdirectory;
Expand Down Expand Up @@ -307,12 +372,16 @@ public void UpdateOptimizerListViewModelList()
continue;
}

List<string> existingDevDriveLetters = ExistingDevDrives.Select(x => x.DriveLetter.ToString()).ToList();

var exampleDirectory = Path.Join(existingDevDriveLetters[0] + ":", cache.ExampleSubDirectory);
Copy link
Contributor

@dhoehna dhoehna Apr 10, 2024

Choose a reason for hiding this comment

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

You sure that existingDevDriveLetters[0] will never be empty? #ByDesign

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, this code never executes if there are no dev drives. We do not display Dev Drive Insights card at all.

var card = new DevDriveOptimizerCardViewModel(
_optimizeDevDriveDialogViewModelFactory,
cache.CacheName!,
existingCacheLocation,
cache.ExampleDirectory!, // example location on dev drive to move cache to
cache.EnvironmentVariable!); // environmentVariableToBeSet
exampleDirectory!, // example location on dev drive to move cache to
cache.EnvironmentVariable!, // environmentVariableToBeSet
existingDevDriveLetters);
DevDriveOptimizerCardCollection.Add(card);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@

using System;
using System.Collections.ObjectModel;
using System.Linq;
using System.Threading.Tasks;
using CommunityToolkit.Mvvm.ComponentModel;
using CommunityToolkit.Mvvm.Input;
using DevHome.Common.Extensions;
using DevHome.Common.Models;
using DevHome.Common.Services;
using Microsoft.UI.Xaml;
using Windows.System;

namespace DevHome.Customization.ViewModels;
Expand Down Expand Up @@ -45,4 +47,6 @@ private void NavigateToDevDriveInsightsPage()
{
NavigationService.NavigateTo(typeof(DevDriveInsightsViewModel).FullName!);
}

public bool AnyDevDrivesPresent => Application.Current.GetService<IDevDriveManager>().GetAllDevDrivesThatExistOnSystem().Any();
}
Loading
Loading