Experimental Feature: Quiet Background Processes UI and WinRT server by jsidewhite · Pull Request #2202 · microsoft/devhome

jsidewhite · 2024-01-31T00:00:44Z

Adding the "Enable 'Quiet Background Processes'" experimental feature to Dev Home. It's a toggle that is enabled similarly to Focus Assist mode, that expires after a period of time. When you enable it/toggle it on via the UI, Dev Home will call into the OS to enable quieting down background services (e.g. Search Indexer).

Requires 'Admin'
Requires OS build 26024 or higher

Summary of the pull request

References and relevant issues

Detailed description of the pull request / Additional comments

Validation steps performed

PR checklist

Closes #xxx
Tests added/passed
Documentation updated

Adding the "Enable 'Quiet Background Processes'" experimental feature to Dev Home. It's a toggle that is enabled similarly to Focus Assist mode, that expires after a period of time. When you enable it/toggle it on via the UI, Dev Home will call into the OS to enable quieting down background services (e.g. Search Indexer). * Requires 'Admin' * Requires OS build 26024 or higher

bbonaby · 2024-02-02T19:06:05Z

settings/DevHome.Settings/Strings/en-us/Resources.resw

  </data>
+  <data name="QuietBackgroundProcessesExperiment_Description" xml:space="preserve">
+    <value>Silence and track background processes that may hinder device performance</value>
+  </data>


we usually add a comment to the resource so the localization folks have more context.

bbonaby · 2024-02-02T19:08:10Z

tools/Experiments/src/Strings/en-us/Resources.resw

+
+    mimetype: application/x-microsoft.net.object.bytearray.base64
+    value   : The object must be serialized into a byte array 
+            : using a System.ComponentModel.TypeConverter


we can remove these comments

bbonaby · 2024-02-02T19:08:58Z

tools/Experiments/src/Strings/en-us/Resources.resw

+    <value>This feature can be activated for 2 hours.</value>
+  </data>
+  <data name="QuietBackgroundProcessesExplanation.Header" xml:space="preserve">
+    <value>Quiet background processes beta feature description</value>


quick question, did a content writer right these descriptions?

bbonaby · 2024-02-02T19:10:57Z

tools/Experiments/src/ViewModels/QuietBackgroundProcessesViewModel.cs

+        _zero = new TimeSpan(0, 0, 0);
+
+        var osVersion = Environment.OSVersion;
+        _validOsVersion = osVersion.Version.Build >= 26024;


are there any plans to backport it? I remember for Dev Drives we had to get the storage folks to write an api that we can check against since version checking wasn't ideal.

bbonaby · 2024-02-02T19:13:35Z

tools/Experiments/src/ViewModels/QuietBackgroundProcessesViewModel.cs

+
+    private string _timeLeft = string.Empty;
+
+    public string TimeLeft


checkout https://learn.microsoft.com/en-us/dotnet/communitytoolkit/mvvm/ its used everywhere in DevHome and by adding attributes about a field it auto generates this code for you. E.g

devhome/settings/DevHome.Settings/ExperimentalFeature.cs

Line 40 in 45b6782

[ObservableProperty]

Using [ObservableProperty takes a bit of extra work at first, but the payoff is less code in code-behind.

bbonaby · 2024-02-02T19:22:37Z

tools/Experiments/src/Views/QuietBackgroundProcessesPage.xaml

+                        </controls:SettingsCard>
+
+                        <!-- Hyperlinks -->
+                        <!--


same as above

bbonaby · 2024-02-02T19:36:00Z

...rocesses.ElevatedServer/DevHome.QuietBackgroundProcesses.QuietBackgroundProcessesManager.idl

+// Copyright (c) Microsoft Corporation and Contributors
+// Licensed under the MIT license.
+
+namespace DevHome.QuietBackgroundProcesses


whats the plan for this idl? Is this a Dev Home internal thing or do we expect people in the public to want to use this too? Just curious is all. I thought with c++/winrt we can use the implements stuct. Or does that not work here so we need to use an idl file?

bbonaby · 2024-02-02T19:36:54Z

tools/QuietBackgroundProcesses/DevHome.QuietBackgroundProcesses.ElevatedServer/Timer.cpp

+    // Destruct time window on sepearate thread because its destructor may take time to end (the std::future member is blocking)
+    // 
+    // (Make a new discard thread and chain the existing one to it)
+    g_discardThread = std::thread([timer = std::move(timer), previousThread = std::move(previousThread)]() mutable {


do we care that std::thread can throw?

bbonaby · 2024-02-02T19:38:27Z

tools/QuietBackgroundProcesses/DevHome.QuietBackgroundProcesses.ElevatedServer/Timer.cpp

+        previousThread = std::move(g_discardThread);
+    }
+
+    // Destruct time window on sepearate thread because its destructor may take time to end (the std::future member is blocking)


Suggested change

// Destruct time window on sepearate thread because its destructor may take time to end (the std::future member is blocking)

// Destruct time window on separate thread because its destructor may take time to end (the std::future member is blocking)

bbonaby · 2024-02-02T19:46:49Z

tools/QuietBackgroundProcesses/DevHome.QuietBackgroundProcesses.ElevatedServer/Timer.h

+
+using CallbackFunction = void (*)();
+
+class Timer


could we have used a periodic timer instead of this timer class? https://learn.microsoft.com/en-us/uwp/api/windows.system.threading.threadpooltimer.createperiodictimer?view=winrt-22621

jsidewhite · 2024-02-02T21:01:07Z

krschau · 2024-02-06T22:33:32Z

src/DevHome.csproj

            <PrivateAssets>all</PrivateAssets>
            <IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
        </PackageReference>
+        <PackageReference Include="Microsoft.Windows.CsWinRT" Version="2.0.4" />


You shouldn't need to add this since we get it for free from DevHome.Common.

krschau · 2024-02-06T22:33:46Z

src/DevHome.csproj

            <ExcludeAssets>contentFiles</ExcludeAssets>
        </PackageReference>
    </ItemGroup>
-


Nit: put this blank line back.

krschau · 2024-02-06T22:34:55Z

src/NavConfig.jsonc

            "icon": "ea86"
          }
-          /*,
+          ,


Nit: put this comma on line above.

krschau · 2024-02-06T22:35:12Z

src/NavConfig.jsonc

-  "ExperimentalFeature_1"
-  */ ]
+  "experiments": [
+  "QuietBackgroundProcessesExperiment"


Nit: indentation

krschau · 2024-02-06T22:35:37Z

src/Package.appxmanifest

@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="utf-8"?>
-<Package xmlns="http://schemas.microsoft.com/appx/manifest/foundation/windows10" xmlns:uap="http://schemas.microsoft.com/appx/manifest/uap/windows10" xmlns:uap3="http://schemas.microsoft.com/appx/manifest/uap/windows10/3" xmlns:rescap="http://schemas.microsoft.com/appx/manifest/foundation/windows10/restrictedcapabilities" xmlns:genTemplate="http://schemas.microsoft.com/appx/developer/templatestudio" xmlns:com="http://schemas.microsoft.com/appx/manifest/com/windows10" xmlns:desktop="http://schemas.microsoft.com/appx/manifest/desktop/windows10" xmlns:desktop6="http://schemas.microsoft.com/appx/manifest/desktop/windows10/6" IgnorableNamespaces="uap uap3 rescap genTemplate desktop6">
+<Package xmlns="http://schemas.microsoft.com/appx/manifest/foundation/windows10" xmlns:uap="http://schemas.microsoft.com/appx/manifest/uap/windows10" xmlns:uap3="http://schemas.microsoft.com/appx/manifest/uap/windows10/3" xmlns:uap5="http://schemas.microsoft.com/appx/manifest/uap/windows10/5" xmlns:rescap="http://schemas.microsoft.com/appx/manifest/foundation/windows10/restrictedcapabilities" xmlns:genTemplate="http://schemas.microsoft.com/appx/developer/templatestudio" xmlns:com="http://schemas.microsoft.com/appx/manifest/com/windows10" xmlns:desktop="http://schemas.microsoft.com/appx/manifest/desktop/windows10" xmlns:desktop6="http://schemas.microsoft.com/appx/manifest/desktop/windows10/6" IgnorableNamespaces="uap uap3 uap5 rescap genTemplate desktop6">


Check that you're not changing the line endings here to LF.

krschau · 2024-02-06T22:40:37Z

tools/Experiments/src/ViewModels/QuietBackgroundProcessesViewModel.cs

+
+        if (!_validOsVersion)
+        {
+            TimeLeft = "This feature requires OS version 10.0.26024.0+";


Should this be localized?

krschau · 2024-02-06T22:46:08Z

tools/Experiments/src/ViewModels/QuietBackgroundProcessesViewModel.cs

+// Copyright (c) Microsoft Corporation and Contributors
+// Licensed under the MIT license.


Headers changed, if you merged main this should helpfully cause an error now. C++ files won't cause an error, but those will need to be addressed as well.

EricJohnson327 · 2024-02-06T22:57:48Z

tools/Experiments/src/DevHome.Experiments.csproj

    <UseWinUI>true</UseWinUI>
  </PropertyGroup>
  <ItemGroup>
+    <None Remove="Views\QuietBackgroundProcessesPage.xaml" />


The QuiteBackgroundProcesses stuff should go in its own tools/QuietBackgroundProcesses project. All code should be written as if it wasn't an experimental feature, then the experimental feature checks just wrap it to enable/disable it. That way when it's not experimental anymore, we just remove the checks and the rest of the code stays exactly the same.

EricJohnson327 · 2024-02-06T22:59:38Z

...rocesses.ElevatedServer/DevHome.QuietBackgroundProcesses.QuietBackgroundProcessesManager.idl

+
+namespace DevHome.QuietBackgroundProcesses
+{
+    [default_interface] runtimeclass QuietBackgroundProcessesManager


Since the namespace is already "QuietBackgroundProcesses", the class can just be "Manager" or "QBPManager" if you really want to differentiate it.

I had that originally, then I noticed that all the APIs keep the full name.

e.g. Windows.ApplicationModel.AppService has AppServiceConnection class

https://learn.microsoft.com/en-us/uwp/api/windows.applicationmodel.appservice.appserviceconnection?view=winrt-22621

I think this is done so that all the using statements don't bring in multiple classes called something generic like "Manager" which then conflict.

personally i agree with your approach though.

manodasanW · 2024-02-07T00:39:36Z

...tedServer.Projection/nuget/DevHome.QuietBackgroundProcesses.ElevatedServer.Projection.nuspec

+    <description>An out-of-proc server for the QuietBackgroundProcesses feature.</description>
+    <dependencies>
+      <group targetFramework="net6.0-windows10.0.19041.0" />
+      <group targetFramework=".NETCoreApp3.0" />


Do you need all these target frameworks for downlevel?

Same for the winmds below.

manodasanW · 2024-02-07T00:40:03Z

...tedServer.Projection/nuget/DevHome.QuietBackgroundProcesses.ElevatedServer.Projection.nuspec

+    <file src="..\..\DevHome.QuietBackgroundProcesses.ElevatedServer\x64\Release\Merged\DevHome.QuietBackgroundProcesses.winmd" target="lib\uap10.0\DevHome.DevHome.QuietBackgroundProcesses.winmd" />
+    <file src="..\..\DevHome.QuietBackgroundProcesses.ElevatedServer\x64\Release\Merged\DevHome.QuietBackgroundProcesses.winmd" target="lib\net46\DevHome.DevHome.QuietBackgroundProcesses.winmd" />
+    <!-- Runtime -->
+    <file src="..\..\..\..\src\bin\x64\Release\net8.0-windows10.0.22000.0\DevHome.QuietBackgroundProcesses.ElevatedServer.exe" target="runtimes\win10-x64\native\DevHome.QuietBackgroundProcesses.ElevatedServer.exe" />


Suggested change

<file src="..\..\..\..\src\bin\x64\Release\net8.0-windows10.0.22000.0\DevHome.QuietBackgroundProcesses.ElevatedServer.exe" target="runtimes\win10-x64\native\DevHome.QuietBackgroundProcesses.ElevatedServer.exe" />

<file src="..\..\..\..\src\bin\x64\Release\net8.0-windows10.0.22000.0\DevHome.QuietBackgroundProcesses.ElevatedServer.exe" target="runtimes\win-x64\native\DevHome.QuietBackgroundProcesses.ElevatedServer.exe" />

Note that this reflects the change done for .NET 8 RID breaking change.

manodasanW · 2024-02-07T01:11:36Z

...evatedServer.Projection/Properties/QuietBackgroundProcesses.ElevatedServer.Projection.rd.xml

@@ -0,0 +1,33 @@
+<?xml version="1.0" encoding="utf-8"?>
+<!--


I think you can drop this file.

manodasanW · 2024-02-07T01:14:03Z

...etBackgroundProcesses.ElevatedServer/DevHome.QuietBackgroundProcesses.ElevatedServer.vcxproj

+    <WholeProgramOptimization>true</WholeProgramOptimization>
+    <LinkIncremental>false</LinkIncremental>
+  </PropertyGroup>
+  <PropertyGroup Label="Configuration" Condition="'$(Configuration)|$(Platform)'=='Release|x64'">


We should set DesktopCompatible to true unconditional on what configuration / platform that it is being built for given we want it for all configurations.

manodasanW · 2024-02-07T01:17:05Z

tools/QuietBackgroundProcesses/DevHome.QuietBackgroundProcesses.ElevatedServer/main.cpp

+        RETURN_HR_IF(E_UNEXPECTED, wil::compare_string_ordinal(WindowsGetStringRawBuffer(name, nullptr), L"DevHome.QuietBackgroundProcesses.QuietBackgroundProcessesManager", true) != 0);
+
+        auto manager = winrt::make<winrt::DevHome::QuietBackgroundProcesses::factory_implementation::QuietBackgroundProcessesManager>();
+        manager.as<winrt::Windows::Foundation::IActivationFactory>();


Doesn't as here return another ComPtr or no? Is this line needed?

manodasanW · 2024-02-07T01:21:27Z

...tedServer.Projection/nuget/DevHome.QuietBackgroundProcesses.ElevatedServer.Projection.nuspec

+    <file src="..\..\DevHome.QuietBackgroundProcesses.ElevatedServer\x64\Release\Merged\DevHome.QuietBackgroundProcesses.winmd" target="lib\uap10.0\DevHome.DevHome.QuietBackgroundProcesses.winmd" />
+    <file src="..\..\DevHome.QuietBackgroundProcesses.ElevatedServer\x64\Release\Merged\DevHome.QuietBackgroundProcesses.winmd" target="lib\net46\DevHome.DevHome.QuietBackgroundProcesses.winmd" />
+    <!-- Runtime -->
+    <file src="..\..\..\..\src\bin\x64\Release\net8.0-windows10.0.22000.0\DevHome.QuietBackgroundProcesses.ElevatedServer.exe" target="runtimes\win10-x64\native\DevHome.QuietBackgroundProcesses.ElevatedServer.exe" />


Do we need ARM64?

bbonaby · 2024-02-07T18:43:57Z

tools/QuietBackgroundProcesses/DevHome.QuietBackgroundProcesses.ElevatedServer/main.cpp

+}
+
+//int _cdecl wmain(int argc, __in_ecount(argc) PWSTR wargv[])
+int CALLBACK wWinMain(_In_ HINSTANCE, _In_ HINSTANCE, _In_ LPWSTR wargv, _In_ int)


Just another question, is the C++ used in this PR needed? Could this have been done via C# instead?

dhoehna · 2024-02-12T22:02:08Z

tools/Experiments/src/ViewModels/QuietBackgroundProcessesViewModel.cs

+        _zero = new TimeSpan(0, 0, 0);
+
+        var osVersion = Environment.OSVersion;
+        _validOsVersion = osVersion.Version.Build >= 26024;


Is comparing against the build number the only way to tell if this feature is on the computer?

dhoehna · 2024-02-12T22:03:01Z

tools/Experiments/src/ViewModels/QuietBackgroundProcessesViewModel.cs

+
+        var osVersion = Environment.OSVersion;
+        _validOsVersion = osVersion.Version.Build >= 26024;
+        _validOsVersion = true;


Why do you set _validOsVersion to true here, when you have it set on line 29?

dhoehna · 2024-02-12T22:04:03Z

tools/Experiments/src/ViewModels/QuietBackgroundProcessesViewModel.cs

+
+        if (!_validOsVersion)
+        {
+            TimeLeft = "This feature requires OS version 10.0.26024.0+";


Why is it called TimeLeft if the string does not mention anything about time?

dhoehna · 2024-02-12T22:06:50Z

tools/Experiments/src/ViewModels/QuietBackgroundProcessesViewModel.cs

+        if (!_validOsVersion)
+        {
+            TimeLeft = "This feature requires OS version 10.0.26024.0+";
+            return;


Returning early is not ideal. Users of this constructor might think that QuietObjectViewModel is in a correct state even if the os version isn't correct. I recommend moving the OS version to methods that determine if QuietMode is supported.

dhoehna · 2024-02-12T22:07:32Z

tools/Experiments/src/ViewModels/QuietBackgroundProcessesViewModel.cs

+        {
+            if (!_isElevated)
+            {
+                TimeLeft = "This feature requires running as admin";


Is TimeLeft an error string?

dhoehna · 2024-02-12T22:08:43Z

tools/Experiments/src/ViewModels/QuietBackgroundProcessesViewModel.cs

+        {
+            _isToggleOn = true;
+            var timeLeftInSeconds = GetTimeRemaining();
+            StartCountdownTimer(timeLeftInSeconds);


is StartCountdownTimer synchronous? Won't this halt execution until the timer is done?

dhoehna · 2024-02-12T22:10:21Z

tools/Experiments/src/ViewModels/QuietBackgroundProcessesViewModel.cs

+    private DispatcherTimer _dispatcherTimer;
+    private TimeSpan _secondsLeft;
+
+    private void StartCountdownTimer(long timeLeftInSeconds)


Consider using TimeSpan to durations instead of long/int

dhoehna · 2024-02-12T22:13:13Z

tools/Experiments/src/Views/QuietBackgroundProcessesPage.xaml

+
+                    <!-- Toggle switch -->
+                    <StackPanel Orientation="Horizontal">
+                        <TextBlock Text="{x:Bind ViewModel.TimeLeft, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}" FontSize="16" Foreground="LightBlue"  Margin="{StaticResource MediumLeftRightMargin}" Grid.Column="1" VerticalAlignment="Center" TextTrimming="CharacterEllipsis" />


Why does the TextBox use Grid.Column? Shouldn't that go into the StackPanel on line 29?

dhoehna · 2024-02-12T22:14:16Z

tools/Experiments/src/Views/QuietBackgroundProcessesPage.xaml

+                    <!-- Toggle switch -->
+                    <StackPanel Orientation="Horizontal">
+                        <TextBlock Text="{x:Bind ViewModel.TimeLeft, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}" FontSize="16" Foreground="LightBlue"  Margin="{StaticResource MediumLeftRightMargin}" Grid.Column="1" VerticalAlignment="Center" TextTrimming="CharacterEllipsis" />
+                        <StackPanel Orientation="Horizontal" HorizontalAlignment="Right">


IF the only thing inside this StackPanel is a ToggleSwitch the StackPanel can be removed.

dhoehna · 2024-02-12T22:14:51Z

tools/Experiments/src/Views/QuietBackgroundProcessesPage.xaml.cs

+/// </summary>
+public sealed partial class QuietBackgroundProcessesPage : ToolPage
+{
+    public override string ShortName => "Quiet Background Processes";


Does this string need to be localized?

dhoehna · 2024-02-12T22:17:45Z

tools/QuietBackgroundProcesses/DevHome.QuietBackgroundProcesses.ElevatedServer/Timer.h

+    void TimerThread()
+    {
+        // Pause until timer expired or cancelled
+        while (true)


is it possible to convert this to a do/while? Looks like you want some code to execute before the exit condition.

krschau · 2024-02-28T16:49:45Z

Closed in favor of #2322 (@jsidewhite please change if this is incorrect)

jsidewhite requested review from AmelBawa-msft, dhoehna, huzaifa-d and krschau February 1, 2024 23:01

bbonaby reviewed Feb 2, 2024

View reviewed changes

krschau reviewed Feb 6, 2024

View reviewed changes

EricJohnson327 reviewed Feb 6, 2024

View reviewed changes

manodasanW reviewed Feb 7, 2024

View reviewed changes

bbonaby reviewed Feb 7, 2024

View reviewed changes

dhoehna reviewed Feb 12, 2024

View reviewed changes

krschau closed this Feb 28, 2024

jsidewhite mentioned this pull request Apr 2, 2024

Move QuietBackgroundProcesses UI to Windows customization #2511

Merged

3 tasks


		private string _timeLeft = string.Empty;

		public string TimeLeft

	// Destruct time window on sepearate thread because its destructor may take time to end (the std::future member is blocking)
	// Destruct time window on separate thread because its destructor may take time to end (the std::future member is blocking)

		// Copyright (c) Microsoft Corporation and Contributors
		// Licensed under the MIT license.

	<file src="..\..\..\..\src\bin\x64\Release\net8.0-windows10.0.22000.0\DevHome.QuietBackgroundProcesses.ElevatedServer.exe" target="runtimes\win10-x64\native\DevHome.QuietBackgroundProcesses.ElevatedServer.exe" />
	<file src="..\..\..\..\src\bin\x64\Release\net8.0-windows10.0.22000.0\DevHome.QuietBackgroundProcesses.ElevatedServer.exe" target="runtimes\win-x64\native\DevHome.QuietBackgroundProcesses.ElevatedServer.exe" />

Conversation

jsidewhite commented Jan 31, 2024

Summary of the pull request

References and relevant issues

Detailed description of the pull request / Additional comments

Validation steps performed

PR checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbonaby Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsidewhite commented Feb 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manodasanW Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbonaby Feb 2, 2024 •

edited

Loading

manodasanW Feb 7, 2024 •

edited

Loading