Dev drive insights and pip cache move rebased to changes from main #2486

SohamDas2021 · 2024-03-29T07:20:00Z

Summary of the pull request

Windows customization root page:

Dev Drive Insights: Listing the dev drives and the Pip cache optimizer card.

Folder picker content dialog to move the cache to dev drive location:

User inputs directory on dev drive:

Pip cache optimized card.

Currently the app needs to be relaunched for reflecting the change.

References and relevant issues

Detailed description of the pull request / Additional comments

Added Dev Drive Insights. User can move pip cache from non dev drive location to the dev drive for optimized performance.

Validation steps performed

pip install works after the change.

PR checklist

Closes #xxx
Tests added/passed
Documentation updated

…om Main?

krschau · 2024-03-29T15:26:18Z

tools/Customization/DevHome.Customization/ViewModels/Environment/DevDriveCardViewModel.cs

+        DevDriveSize = devDrive.DriveSizeInBytes / divider;
+        DevDriveFreeSize = devDrive.DriveSizeRemainingInBytes / divider;
+        DevDriveUsedSize = DevDriveSize - DevDriveFreeSize;
+        DevDriveUnitOfMeasure = (devDrive.DriveUnitOfMeasure == ByteUnit.TB) ? "TB" : "GB";


These probably have to be localized. #WontFix

These are units of data. Do we localize these elsewhere?

tools/Customization/DevHome.Customization/ViewModels/Environment/DevDriveCardViewModel.cs

krschau · 2024-03-29T15:29:18Z

...Customization/DevHome.Customization/ViewModels/Environment/DevDriveOptimizerCardViewModel.cs

+        {
+            var optimizeDevDriveDialog = new OptimizeDevDriveDialog(CacheToBeMoved, ExistingCacheLocation, EnvironmentVariableToBeSet);
+            optimizeDevDriveDialog.XamlRoot = settingsCard.XamlRoot;
+            optimizeDevDriveDialog.RequestedTheme = settingsCard.ActualTheme;


I don't think you want to do this -- it will use the right theme without it, and if the RequestedTheme is Default, then this will cause a bug when the System theme is changed. For example, if RequestedTheme is Default and ActualTheme is Dark, but then you set the system theme to Light, the theme of the dialog will stay Dark #Pending

Without this, I see light theme dialog pop up for a dark themed app.

Does it make a difference if you include the Style on the dialog like I mentioned below? There should be a way we can remove this.

I have the following:

Style="{StaticResource DefaultContentDialogStyle}" in the dialog.

That is not making any difference- still seeing light theme dialog with dark theme app without setting the RequestedTheme, if that is what you are referring to.

krschau · 2024-03-29T15:30:37Z

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

+    behaviors:NavigationViewHeaderBehavior.HeaderMode="Never"
+    mc:Ignorable="d">
+
+    <Grid MaxWidth="{ThemeResource MaxPageContentWidth}" Margin="{ThemeResource ContentPageMargin}">


This page as it currently is will have this bug: #566 #Resolved

I am guessing removing the BreadcrumbBar will take care of this.

Not quite -- you'll have to have a ScrollViewer fill the width, then set the MaxWidth and Margin on whatever's inside the encompassing ScrollViewer.

krschau · 2024-03-29T15:32:07Z

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

+            ItemsSource="{x:Bind Breadcrumbs}" />
+
+        <ScrollView Grid.Row="1" VerticalAlignment="Top">
+            <views:DevDriveInsightsView />


Will there be other views that this might contain in the future? Why separate the page and view? #ByDesign

Yes there could be other views to be contained here in future.

tools/Customization/DevHome.Customization/Views/OptimizeDevDriveDialog.xaml

krschau · 2024-03-29T15:50:36Z

tools/Customization/DevHome.Customization/Views/OptimizeDevDriveDialog.xaml.cs

+        OptimizeDevDriveDialogDescription = stringResource.GetLocalized("OptimizeDevDriveDialogDescription", ExistingCacheLocation, EnvironmentVariableToBeSet);
+    }
+
+    public string CacheToBeMoved


As Branden mentioned elsewhere, these DPs should all just be ObservableProperties. #Resolved

krschau · 2024-04-01T14:49:19Z

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

@@ -117,6 +117,10 @@
  <resheader name="writer">
    <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
  </resheader>
+  <data name="ChooseDirectoryPromptText" xml:space="preserve">
+    <value>Choose directory on dev drive...</value>


Dev Drive should be capitalized, I believe. (Applies to all instances on this page)

Suggested change

<value>Choose directory on dev drive...</value>

<value>Choose directory on Dev Drive...</value>

``` #Resolved

That is what it says in the Figma.

actually @SohamDas2021 it should be like Kristen outlined, If I recall, it was from Marketing who said that.

So capitalize all Dev Drive in the text?

Fixed, thanks.

krschau · 2024-04-01T14:51:03Z

...ustomization/DevHome.Customization/ViewModels/Environment/OptimizeDevDriveDialogViewModel.cs

+    private async Task BrowseButtonClick(object sender)
+    {
+        // Create a folder picker dialog
+        var folderPicker = new FolderPicker


Should this be using @AmelBawa-msft 's new Picker APIs? If this is ever launched from admin, it definitely should. #Resolved

I have noted this down as a suggestion and will do it as a bug fix later. Will be opening issue to track this. Trying to complete this today.

krschau · 2024-04-01T14:53:48Z

...ustomization/DevHome.Customization/ViewModels/Environment/OptimizeDevDriveDialogViewModel.cs

+    private string _exampleDevDriveLocation;
+
+    [ObservableProperty]
+    private string _chooseDirectoryPromptText;


Rather than saving all these strings in the ViewModel, you should reference them from the xaml markup directly. See https://learn.microsoft.com/en-us/windows/apps/windows-app-sdk/mrtcore/localize-strings#refer-to-a-string-resource-identifier-from-xaml #WontFix

Many of these strings have dynamic bindings-
Contents of {0} will be copied to chosen directory. And {1} will be set to chosen directory.

Can't seem to find a way of doing this through the xaml. Won't fixing for now. Keeping a note and will be opening an issue to track.

krschau · 2024-04-01T14:55:44Z

tools/Customization/DevHome.Customization/DevHome.Customization.csproj

@@ -24,6 +24,8 @@

  <ItemGroup>
    <ProjectReference Include="..\..\..\common\DevHome.Common.csproj" />
+	<ProjectReference Include="..\..\SetupFlow\DevHome.SetupFlow.Common\DevHome.SetupFlow.Common.csproj" />


Would be great if you could add the DevHome.Customization project to the mermaid diagram of project relationships in https://github.com/microsoft/devhome/blob/main/docs/architecture.md #Resolved

krschau · 2024-04-01T14:59:37Z

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

+    public DevDriveInsightsViewModel(IDevDriveManager devDriveManager, OptimizeDevDriveDialogViewModelFactory optimizeDevDriveDialogViewModelFactory)
+    {
+        _optimizeDevDriveDialogViewModelFactory = optimizeDevDriveDialogViewModelFactory;
+        _dispatcher = Microsoft.UI.Dispatching.DispatcherQueue.GetForCurrentThread();


If this isn't running on the UI thread, this will return null. Use WindowEx instead.

Running on UI thread.

It isn't guaranteed that this will run on the UI thread every time. I can forward you the internal thread I started about this. This can return null and will result in a crash.

krschau · 2024-04-01T15:01:53Z

tools/Customization/DevHome.Customization/Views/DevDriveInsightsView.xaml

+    xmlns:converters="using:CommunityToolkit.WinUI.Converters"
+    xmlns:ctControls="using:CommunityToolkit.WinUI.Controls"
+    mc:Ignorable="d"
+    Loaded="UserControl_Loaded">


You should bind this to a RelayCommand directly on the ViewModel, so that you don't need the intermediate event handler on the View. It will also prevent any concurrency issues if a user were to switch between pages really quickly. #WontFix

krschau · 2024-04-01T15:03:01Z

tools/Customization/DevHome.Customization/Views/OptimizeDevDriveDialog.xaml

+    <StackPanel>
+        <TextBlock Text="{x:Bind ViewModel.OptimizeDevDriveDialogDescription}" TextWrapping="Wrap"/>
+        <Grid Margin="0 10 0 0">
+            <TextBox HorizontalAlignment="Left" Width="300" Grid.Column="0" x:Name="DirectoryPathTextBox" PlaceholderText="Choose a directory" Text="{x:Bind ViewModel.DirectoryPathTextBox , Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}"/>


Suggest using MinWidth, so that it can scale up in size if text scale is turned up (preventing accessibility bug) #Resolved

nieubank · 2024-04-01T18:54:21Z

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

+    xmlns:d="https://schemas.microsoft.com/expression/blend/2008"
+    xmlns:mc="https://schemas.openxmlformats.org/markup-compatibility/2006"
+    xmlns:views="using:DevHome.Customization.Views"
+    behaviors:NavigationViewHeaderBehavior.HeaderMode="Always"


Always

You add the breadcrumb logic now -

see #2501 #Resolved

I will do it as a separate PR once I check this in.

bbonaby

Thanks for working on this and getting it into its current state. I don't think I've seen anything that would block the check in

bbonaby · 2024-04-01T20:16:09Z

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

+    <comment>Dev drive size free</comment>
+  </data>
+  <data name="DevDriveInsightsCard.Description" xml:space="preserve">
+    <value>All things, dev drives, optimizations, etc.</value>


Missed this one, but will let you update it in another PR #Resolved

bbonaby · 2024-04-01T20:16:44Z

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

+        if (directoryPath != null)
+        {
+            // Handle the selected folder
+            // TODO: If chosen folder not a dev drive location, currently we no-op. Instead we should display the error.


Do we have an issue on GitHub for this? #Closed

Yet to create the issues, will do.

SohamDas2021 · 2024-04-01T20:29:56Z

Thanks for approving.

In reply to: 1972049143

nieubank

Applied Dev Drive Insights changes on top of Windows Customization fr…

6058ab8

…om Main?

SohamDas2021 marked this pull request as ready for review March 29, 2024 07:21

SohamDas2021 requested review from dhoehna, krschau and bbonaby March 29, 2024 07:22

This was referenced Mar 29, 2024

Dev drive insights and pip cache move #2433

Closed

List Dev Drives from Setup flow and associated changes #2397

Closed

krschau reviewed Mar 29, 2024

View reviewed changes

SohamDas2021 added 2 commits March 29, 2024 12:02

Addressed PR comments 1

1a0d87f

Addressed PR comments

aa482b9

krschau reviewed Apr 1, 2024

View reviewed changes

Addressed PR comments

f162c12

nieubank reviewed Apr 1, 2024

View reviewed changes

More cleanups

a1a730a

bbonaby approved these changes Apr 1, 2024

View reviewed changes

SohamDas2021 changed the base branch from microsoftfeature/dev-drive-insights-new to main April 1, 2024 20:32

Merge branch 'main' into user/sodas/devdriveinsights_032824

54fd0cf

kanismohammed self-requested a review April 1, 2024 20:36

kanismohammed approved these changes Apr 1, 2024

View reviewed changes

nieubank approved these changes Apr 1, 2024

View reviewed changes

SohamDas2021 merged commit f4e9a64 into main Apr 1, 2024
4 checks passed

	<value>Choose directory on dev drive...</value>
	<value>Choose directory on Dev Drive...</value>
	``` #Resolved

Dev drive insights and pip cache move rebased to changes from main #2486

Dev drive insights and pip cache move rebased to changes from main #2486

Conversation

SohamDas2021 commented Mar 29, 2024

Summary of the pull request

References and relevant issues

Detailed description of the pull request / Additional comments

Validation steps performed

PR checklist

krschau Mar 29, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krschau Mar 29, 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

SohamDas2021 Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

krschau Mar 29, 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

krschau Mar 29, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krschau Mar 29, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

krschau Apr 1, 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 1, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

SohamDas2021 Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

krschau Apr 1, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krschau Apr 1, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krschau Apr 1, 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

krschau Apr 1, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

krschau Apr 1, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

nieubank Apr 1, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbonaby left a comment

Choose a reason for hiding this comment

bbonaby Apr 1, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

bbonaby Apr 1, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SohamDas2021 commented Apr 1, 2024

nieubank left a comment

Choose a reason for hiding this comment

krschau Mar 29, 2024 •

edited by SohamDas2021

Loading

krschau Mar 29, 2024 •

edited by SohamDas2021

Loading

SohamDas2021 Apr 1, 2024 •

edited

Loading

krschau Mar 29, 2024 •

edited by SohamDas2021

Loading

krschau Mar 29, 2024 •

edited by SohamDas2021

Loading

krschau Mar 29, 2024 •

edited by SohamDas2021

Loading

krschau Apr 1, 2024 •

edited by SohamDas2021

Loading

krschau Apr 1, 2024 •

edited by SohamDas2021

Loading

SohamDas2021 Apr 1, 2024 •

edited

Loading

krschau Apr 1, 2024 •

edited by SohamDas2021

Loading

krschau Apr 1, 2024 •

edited by SohamDas2021

Loading

krschau Apr 1, 2024 •

edited by SohamDas2021

Loading

krschau Apr 1, 2024 •

edited by SohamDas2021

Loading

krschau Apr 1, 2024 •

edited by SohamDas2021

Loading

nieubank Apr 1, 2024 •

edited by SohamDas2021

Loading

bbonaby Apr 1, 2024 •

edited by SohamDas2021

Loading

bbonaby Apr 1, 2024 •

edited by SohamDas2021

Loading