More caches added, other improvements #2551

SohamDas2021 · 2024-04-08T17:28:08Z

Summary of the pull request

Adding more package caches listed in https://learn.microsoft.com/en-us/windows/dev-drive/#storing-package-cache-on-dev-drive for Dev Drive optimization.
The PR also consists of the following improvements:

Partial auto refresh. After optimizing a cache, the user can return to Dev Drive insights and see the change reflected.
Visibility on dev drive insights card is made conditional on the presence of dev drives.
Breadcrumb bar back on
Telemetry for Data team

References and relevant issues

Detailed description of the pull request / Additional comments

Validation steps performed

PR checklist

Closes #xxx
Tests added/passed
Documentation updated

…mb back, telemetry, cleanups

SohamDas2021 · 2024-04-08T21:24:57Z

...ization/DevHome.Customization/ViewModels/DevDriveInsights/OptimizeDevDriveDialogViewModel.cs

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


// 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

dhoehna · 2024-04-09T17:38:14Z

tools/Customization/DevHome.Customization/Extensions/ServiceExtensions.cs

@@ -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));


What is this doing? #Closed

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

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

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 :)

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.

dhoehna · 2024-04-09T17:39:25Z

tools/Customization/DevHome.Customization/Strings/en-us/Resources.resw

-    <comment>Example dev drive location</comment>
+  <data name="ExampleText" xml:space="preserve">
+    <value>Example: </value>
+    <comment>Example string</comment>


NIT: Add more to the comments to differentiate this from an example. I thought this was a sample copy and pasted from online. #Resolved

dhoehna · 2024-04-09T17:42:19Z

...ization/DevHome.Customization/ViewModels/DevDriveInsights/OptimizeDevDriveDialogViewModel.cs

        }
        catch (Exception ex)
        {
            Log.Error($"Error in MoveDirectory. Error: {ex}");
+            TelemetryFactory.Get<ITelemetry>().LogError("DevDriveInsights_PackageCacheMove_Error", LogLevel.Critical, new ExceptionEvent(ex.HResult, sourceDirectory));


This event is Package CacheMove. Another event below is PackageCacheMoved I suggest to Think of event names that differ more than 1 letter. #Resolved

dhoehna · 2024-04-09T17:46:33Z

...ization/DevHome.Customization/ViewModels/DevDriveInsights/OptimizeDevDriveDialogViewModel.cs

@@ -78,7 +91,7 @@ private async Task BrowseButtonClick(object sender)
        }
    }

-    private void MoveDirectory(string sourceDirectory, string targetDirectory)
+    private int MoveDirectory(string sourceDirectory, string targetDirectory)


Please don't use the C++ return HResult pattern. Replace MoveDirectory with Directory.Move

Also, MoveDirectory handled the error. The error code shouldn't be returned. #Resolved

Directory.Move cannot handle moving between drives.

I am using the error code in the caller. What is the alternative here other than using HResult pattern?

I think the normal C# way would be to return a bool indicating success. The == 0 below feels very out of place.

dhoehna · 2024-04-09T17:47:59Z

tools/Customization/DevHome.Customization/ViewModels/DevDriveInsightsViewModel.cs

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

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

+    private const string ExampleDevDriveDirStr = "D:";


This'll bit a bit awkward if the example shows D: but the user is already ising their D: drive. #Resolved

This is a valid point. Let me switch to an existing dev drive.

tools/Customization/DevHome.Customization/ViewModels/DevDriveInsightsViewModel.cs

dhoehna · 2024-04-09T17:49:59Z

tools/Customization/DevHome.Customization/ViewModels/DevDriveInsightsViewModel.cs

+        },
+        new DevDriveCacheData
+        {
+            CacheName = "Npm cache (NodeJS)",


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

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.

Does this need localization?

Does this need localization?

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

krschau · 2024-04-09T20:14:35Z

tools/Customization/DevHome.Customization/Strings/en-us/Resources.resw

-    <value>Example: E:\packages\pip</value>
-    <comment>Example dev drive location</comment>
+  <data name="ExampleText" xml:space="preserve">
+    <value>Example: </value>


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

It is getting reflected.

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

krschau · 2024-04-09T20:17:11Z

...ization/DevHome.Customization/ViewModels/DevDriveInsights/OptimizeDevDriveDialogViewModel.cs

@@ -78,7 +91,7 @@ private async Task BrowseButtonClick(object sender)
        }
    }

-    private void MoveDirectory(string sourceDirectory, string targetDirectory)
+    private int MoveDirectory(string sourceDirectory, string targetDirectory)


I think the normal C# way would be to return a bool indicating success. The == 0 below feels very out of place.

krschau · 2024-04-09T20:18:50Z

tools/Customization/DevHome.Customization/ViewModels/DevDriveInsightsViewModel.cs

+        },
+        new DevDriveCacheData
+        {
+            CacheName = "Npm cache (NodeJS)",


Does this need localization?

krschau · 2024-04-09T20:24:34Z

tools/Customization/DevHome.Customization/Views/DevDriveInsightsPage.xaml

-    behaviors:NavigationViewHeaderBehavior.HeaderMode="Always"
-    mc:Ignorable="d">
+    behaviors:NavigationViewHeaderBehavior.HeaderTemplate="{StaticResource BreadcrumbBarDataTemplate}"
+    behaviors:NavigationViewHeaderBehavior.HeaderContext="{x:Bind ViewModel}">

    <Grid MaxWidth="{ThemeResource MaxPageContentWidth}" Margin="{ThemeResource ContentPageMargin}">
        <Grid.RowDefinitions>


You don't need the RowDefinitions (or to set the Grid.Row on the ScrollView) since there's only one row.
This page will also have the #566 bug. #Resolved

tools/Customization/DevHome.Customization/Views/MainPageView.xaml

krschau · 2024-04-09T20:39:22Z

Please add some information to the description on this PR. #Resolved

SohamDas2021 · 2024-04-09T20:54:03Z

Added. Thx.

In reply to: 2046011290

tools/Customization/DevHome.Customization/Views/DevDriveInsightsPage.xaml

dhoehna · 2024-04-10T17:17:41Z

...ization/DevHome.Customization/ViewModels/DevDriveInsights/OptimizeDevDriveDialogViewModel.cs

@@ -78,7 +91,7 @@ private async Task BrowseButtonClick(object sender)
        }
    }

-    private void MoveDirectory(string sourceDirectory, string targetDirectory)
+    private bool MoveDirectory(string sourceDirectory, string targetDirectory)


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

dhoehna · 2024-04-10T17:19:47Z

tools/Customization/DevHome.Customization/ViewModels/DevDriveInsightsViewModel.cs

@@ -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);


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

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

dhoehna · 2024-04-10T17:22:57Z

tools/Customization/DevHome.Customization/ViewModels/DevDriveInsightsViewModel.cs

@@ -307,12 +372,20 @@ public void UpdateOptimizerListViewModelList()
                continue;
            }

+            List<string> existingDevDriveLetters = new List<string>();


NIT: existingDevDriveLetters = ExistingDevDrives.Select(x => x.DriveLetter.ToString()).ToList; #Resolved

dhoehna · 2024-04-10T17:23:42Z

tools/Customization/DevHome.Customization/ViewModels/DevDriveInsightsViewModel.cs

+                existingDevDriveLetters.Add(existingDevDrive.DriveLetter.ToString());
+            }
+
+            var exampleDirectory = Path.Join(existingDevDriveLetters[0] + ":", cache.ExampleSubDirectory);


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

Yes, this code never executes if there are no dev drives. We do not display Dev Drive Insights card at all.

SohamDas2021 added 3 commits April 4, 2024 12:23

Partial auto refresh, visibility on dev drive insights card, breadcru…

c4cbef3

…mb back, telemetry, cleanups

More caches optimized

0967d46

Improvements

c243131

SohamDas2021 marked this pull request as ready for review April 8, 2024 21:24

SohamDas2021 requested a review from bbonaby April 8, 2024 21:24

SohamDas2021 commented Apr 8, 2024

View reviewed changes

SohamDas2021 requested a review from dhoehna April 8, 2024 21:27

Added telemetry for Data team

14100ee

dhoehna reviewed Apr 9, 2024

View reviewed changes

tools/Customization/DevHome.Customization/ViewModels/DevDriveInsightsViewModel.cs Show resolved Hide resolved

dhoehna reviewed Apr 9, 2024

View reviewed changes

Addressed PR comments

caeaad9

krschau reviewed Apr 9, 2024

View reviewed changes

Addressed PR comments

8db7405

krschau reviewed Apr 9, 2024

View reviewed changes

tools/Customization/DevHome.Customization/Views/DevDriveInsightsPage.xaml Outdated Show resolved Hide resolved

Fix scrollbar

25b19ec

dhoehna reviewed Apr 10, 2024

View reviewed changes

krschau approved these changes Apr 10, 2024

View reviewed changes

dhoehna reviewed Apr 10, 2024

View reviewed changes

dhoehna approved these changes Apr 10, 2024

View reviewed changes

SohamDas2021 and others added 2 commits April 10, 2024 12:28

Addressed PR comments, removed private info from logs

b908923

Merge branch 'main' into user/sodas/devdriveinsights_morecaches_040424

931b09d

SohamDas2021 merged commit 48d70f2 into main Apr 10, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More caches added, other improvements #2551

More caches added, other improvements #2551

SohamDas2021 commented Apr 8, 2024 •

edited

Loading

SohamDas2021 Apr 8, 2024 •

edited

Loading

dhoehna Apr 9, 2024 •

edited by SohamDas2021

Loading

SohamDas2021 Apr 9, 2024

dhoehna Apr 10, 2024

krschau Apr 10, 2024

dhoehna Apr 10, 2024

dhoehna Apr 9, 2024 •

edited by SohamDas2021

Loading

SohamDas2021 Apr 9, 2024

dhoehna Apr 9, 2024 •

edited by SohamDas2021

Loading

dhoehna Apr 9, 2024 •

edited by SohamDas2021

Loading

SohamDas2021 Apr 9, 2024

krschau Apr 9, 2024

dhoehna Apr 9, 2024 •

edited by SohamDas2021

Loading

SohamDas2021 Apr 9, 2024

dhoehna Apr 9, 2024 •

edited by SohamDas2021

Loading

SohamDas2021 Apr 9, 2024

krschau Apr 9, 2024

dhoehna Apr 10, 2024

krschau Apr 9, 2024 •

edited by SohamDas2021

Loading

SohamDas2021 Apr 9, 2024

krschau Apr 9, 2024

krschau Apr 9, 2024

krschau Apr 9, 2024

krschau Apr 9, 2024 •

edited by SohamDas2021

Loading

krschau commented Apr 9, 2024 •

edited by SohamDas2021

Loading

SohamDas2021 commented Apr 9, 2024

dhoehna Apr 10, 2024 •

edited by SohamDas2021

Loading

dhoehna Apr 10, 2024

krschau Apr 10, 2024

dhoehna Apr 10, 2024 •

edited by SohamDas2021

Loading

dhoehna Apr 10, 2024 •

edited by SohamDas2021

Loading

SohamDas2021 Apr 10, 2024

		@@ -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);

More caches added, other improvements #2551

More caches added, other improvements #2551

Conversation

SohamDas2021 commented Apr 8, 2024 • edited Loading

Summary of the pull request

References and relevant issues

Detailed description of the pull request / Additional comments

Validation steps performed

PR checklist

SohamDas2021 Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

dhoehna Apr 9, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhoehna Apr 9, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhoehna Apr 9, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

dhoehna Apr 9, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhoehna Apr 9, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhoehna Apr 9, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krschau Apr 9, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krschau Apr 9, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

krschau commented Apr 9, 2024 • edited by SohamDas2021 Loading

SohamDas2021 commented Apr 9, 2024

dhoehna Apr 10, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhoehna Apr 10, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

dhoehna Apr 10, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SohamDas2021 commented Apr 8, 2024 •

edited

Loading

SohamDas2021 Apr 8, 2024 •

edited

Loading

dhoehna Apr 9, 2024 •

edited by SohamDas2021

Loading

dhoehna Apr 9, 2024 •

edited by SohamDas2021

Loading

dhoehna Apr 9, 2024 •

edited by SohamDas2021

Loading

dhoehna Apr 9, 2024 •

edited by SohamDas2021

Loading

dhoehna Apr 9, 2024 •

edited by SohamDas2021

Loading

dhoehna Apr 9, 2024 •

edited by SohamDas2021

Loading

krschau Apr 9, 2024 •

edited by SohamDas2021

Loading

krschau Apr 9, 2024 •

edited by SohamDas2021

Loading

krschau commented Apr 9, 2024 •

edited by SohamDas2021

Loading

dhoehna Apr 10, 2024 •

edited by SohamDas2021

Loading

dhoehna Apr 10, 2024 •

edited by SohamDas2021

Loading

dhoehna Apr 10, 2024 •

edited by SohamDas2021

Loading