[Hosts] Backup Settings #37778

davidegiacometti · 2025-03-05T22:18:24Z

Summary of the Pull Request

Add backup settings for the Hosts File Editor to allow users to customize the existing hardcoded logic.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Backup on: verified that backup isn't executed
Backups off: Verified that only one backup is executed
Verified that backup is located in the expected path
Auto delete is set to "never": verified that no backups are deleted
Auto delete is set to "based on count": verified that backups are deleted according to count value
Auto delete is set to "based on age and count": verified that backups are deleted according to days and count values
Verified that files without the backup pattern aren't deleted
There is also adequate test coverage for these scenarios 🚀

src/settings-ui/Settings.UI/Strings/en-us/Resources.resw

htcfreek · 2025-03-06T07:23:36Z

I suggest as we have the on/of checkbox for cleanup to set the number boxes to min=1.

davidegiacometti · 2025-03-06T19:03:02Z

I suggest as we have the on/of checkbox for cleanup to set the number boxes to min=1.

The idea is that user can configure retention by:

Days only: Days = N - Backups = 0
Backups only: Days = 0 - Backups = N
Both: Days = N - Backups = N

In case both are set to 0 the warning is displayed and no backups are deleted.

Do you think this makes sense?

htcfreek

some ui nits

src/settings-ui/Settings.UI/SettingsXAML/Views/HostsPage.xaml

htcfreek · 2025-03-06T19:12:57Z

I suggest as we have the on/of checkbox for cleanup to set the number boxes to min=1.

The idea is that user can configure retention by:

Days only: Days = N - Backups = 0

Backups only: Days = 0 - Backups = N

Both: Days = N - Backups = N

In case both are set to 0 the warning is displayed and no backups are deleted.

Do you think this makes sense?

These all feels to complicated. I think keep it simple is the way to go.

And simple for me is:

Backups are enabled or not.
Automatic cleanup is "disable" or "based on age" or "based on number". (Can be a drop down.)
Based on the cleanup type I can configure nothing or the days or the number.

davidegiacometti · 2025-03-06T19:26:16Z

These all feels to complicated. I think keep it simple is the way to go.

And simple for me is:

Backups are enabled or not.

Automatic cleanup is "disable" or "based on age" or "based on number". (Can be a drop down.)

Based on the cleanup type I can configure nothing or the days or the number.

I started with this idea, but #37666 (comment) made me change it.

Safer would be to keep at least the most recent backup (no matter how old) and then delete others older than 15 days?

But I am happy to have a feedback on this.

htcfreek · 2025-03-06T19:50:36Z

Safer would be to keep at least the most recent backup (no matter how old) and then delete others older than 15 days?\n\nBut I am happy to have a feedback on this.

Okay. But then lets allow the following logic: Remove backups older x days (x = setting >= 1) and hard coded keep always 1 backup.

The if-else-else solution is to complicated.

We can add a description to cleanup checkbox that at least one backup is always kept regardless of its age.

davidegiacometti · 2025-03-06T20:38:08Z

I need to think about this: the idea around the PR is to remove any hardcoded/hidden logic related to backups.
What do you find confusing of the proposed logic? The warning message?

htcfreek · 2025-03-06T21:18:18Z

I need to think about this: the idea around the PR is to remove any hardcoded/hidden logic related to backups.
What do you find confusing of the proposed logic? The warning message?

The point that we have multiple ways of doing it which influence each other. And that you can configure it to not clean up even that zhe clean up is enabled.

davidegiacometti · 2025-03-08T13:08:39Z

@htcfreek any thoughts on this UX?

No warning
User can set only Days or Copies to 0
When Days and Copies are set to 0, "Automatically delete old backups" is disabled and Days and Copies are set to their default

htcfreek · 2025-03-08T13:12:38Z

@htcfreek any thoughts on this UX?

No warning

User can set only Days or Copies to 0

When Days and Copies are set to 0, "Automatically delete old backups" is disabled and Days and Copies are set to their default

@davidegiacometti
Much better.

With zero as date or time all backups will be deleted on the next ????. We should add description to explain that. May on the cleanup check box something like "Clean up is executed on the ....".

davidegiacometti · 2025-03-08T13:31:13Z

The backups cleanup is always executed on editor startup. These options are used to determine for how many days backups are kept or/and how many fixed backups are kept.

htcfreek · 2025-03-08T13:50:02Z

The backups cleanup is always executed on editor startup. These options are used to determine for how many days backups are kept or/and how many fixed backups are kept.

Yes. What I mean is: It makes a difference with 0 days if I start Hosts multiple times per day or not.

And for me keeping 0 days means deleting at the day of creating and not keeping indefinitely. That is confusing.

davidegiacometti · 2025-03-08T15:05:01Z

Yes. What I mean is: It makes a difference with 0 days if I start Hosts multiple times per day or not.

0 days means only the number of backups is considered.

And for me keeping 0 days means deleting at the day of creating and not keeping indefinitely. That is confusing.

You mean that this may be confused as Keep for infinite days AND Keep 10 backups... I see...

TBH I am thinking about reconsider the following solution even if user is forced to choose between number of days or number or backups.

Backups are enabled or not.

Automatic cleanup is "disable" or "based on age" or "based on number". (Can be a drop down.)

Based on the cleanup type I can configure nothing or the days or the number.

htcfreek · 2025-03-08T16:56:38Z

Yes. What I mean is: It makes a difference with 0 days if I start Hosts multiple times per day or not.

0 days means only the number of backups is considered.

And for me keeping 0 days means deleting at the day of creating and not keeping indefinitely. That is confusing.

You mean that this may be confused as Keep for infinite days AND Keep 10 backups... I see...

No. As keeping 0 backups for 10 days which means no backups kept.

To make the current UI logically we need drop downs instead of number boxes:

Keep backups: All, 1, 2, 3, 5, 10, 15, 30, 50, 75, 100
Kepp days: Indefinit, 1, 2, 3, 5, 10, 15, 30, 60, 90, 180

TBH I am thinking about reconsider the following solution even if user is forced to choose between number of days or number or backups.

Backups are enabled or not.

Automatic cleanup is "disable" or "based on age" or "based on number". (Can be a drop down.)

Based on the cleanup type I can configure nothing or the days or the number.

Would be the easiest implementation.

davidegiacometti · 2025-03-08T17:35:03Z

I think the drop downs are limiting.. 🤔
I’m going to close this PR for now and try to create a new one with a better approach in the future.
In any case, it is not a priority but rather an enhancement.

davidegiacometti · 2025-03-08T20:26:57Z

Hey @htcfreek

Can I have a feedback on these mocks?
Every NumberBox has Minimum set to 1, except for Based on age | Number of backups to keep that is optional and has a Minimum of 0.

420622303-349f7a33-355b-4665-b5fe-2abdadb7ccfa

420622309-c4571bca-3dd7-40ab-85bd-a9205c3d0948

Let me know if you think this solution is clear. If your answer is positive I am going to reopen this PR and tweak the implementation.

htcfreek · 2025-03-08T20:47:51Z

@davidegiacometti

Two suggestions:

Replace Number with Count in the drop down.
For the combined type use Based on age and count in the drop down.

Every NumberBox has Minimum set to 1, except for Based on age | Number of backups to keep that is optional and has a Minimum of 0.

Can't follow you. There are only these two number boxes in the screenshots. Are there any screenshots missing?

And I still see see the problem that 0 means nothing. So we should explain the meaning of 0 in the number box description.

davidegiacometti · 2025-03-08T21:00:25Z

Ok for the suggestions.

Please see the update screenshot.
TBH I don't understand what's the problem of 0: see the last NumberBox description.

htcfreek · 2025-03-08T21:45:50Z

TBH I don't understand what's the problem of 0: see the last NumberBox description.

Sory. My mistake. Was confused after all the solutions we discussed.

htcfreek · 2025-03-08T21:47:18Z

Let me know if you think this solution is clear. If your answer is positive I am going to reopen this PR and tweak the implementation.

Yes. It is clear. Let's implement that solution.

davidegiacometti · 2025-03-09T11:18:43Z

Thanks for the feedback.. And I like this solution!
Reopening the PR.

src/settings-ui/Settings.UI/Strings/en-us/Resources.resw

davidegiacometti · 2025-03-10T19:57:57Z

Addressed the last feedback and updated the screenshot in the first post.
I'll take some time to review and test once again before removing the draft from the PR.

Copilot

Pull Request Overview

This PR adds customizable backup settings for the Hosts File Editor, replacing hardcoded backup logic with configurable settings and integrating backup creation/deletion into the Hosts service. Key changes include:

Implementation of a BackupManager with configurable backup creation and deletion modes.
Updates to Hosts settings, view models, and properties to expose backup-related configuration.
Addition of tests and adjustments to the HostsService to integrate the new backup manager functionality.

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/modules/Hosts/HostsUILib/Helpers/BackupManager.cs	Introduces backup creation and deletion logic with new settings support.
src/modules/Hosts/Hosts.Tests/BackupManagerTest.cs	Adds tests for verifying backup creation and deletion behavior.
src/settings-ui/Settings.UI.Library/Enumerations/HostsDeleteBackupMode.cs	Provides new enumeration for backup deletion modes.
src/modules/Hosts/HostsUILib/Settings/HostsDeleteBackupMode.cs	Mirrors the new backup deletion mode enumeration for Hosts.
src/modules/Hosts/HostsUILib/Helpers/IBackupManager.cs	Defines the new IBackupManager interface.
src/settings-ui/Settings.UI/ViewModels/HostsViewModel.cs	Adds new properties and a method to select a backup path.
src/modules/Hosts/Hosts/Settings/UserSettings.cs	Updates default values and JSON loading to include backup settings.
src/settings-ui/Settings.UI.Library/HostsProperties.cs	Adds backup settings default values and properties.
src/modules/Hosts/HostsUILib/Settings/IUserSettings.cs	Extends the user settings interface with backup-related properties.
src/modules/Hosts/Hosts/HostsXAML/App.xaml.cs	Registers the BackupManager and invokes backup deletion on startup.
src/settings-ui/Settings.UI/SettingsXAML/Views/HostsPage.xaml.cs	Updates UI strings for backup settings.
src/modules/Hosts/HostsUILib/Helpers/HostsService.cs	Integrates the new BackupManager in place of previous backup logic.
src/modules/Hosts/Hosts.FuzzTests/FuzzTests.cs, src/modules/Hosts/Hosts.Tests/HostsServiceTest.cs	Adjusts tests to work with the new backup manager integration.

src/modules/Hosts/HostsUILib/Helpers/BackupManager.cs

jamrobot · 2025-03-18T01:26:26Z

src/modules/Hosts/Hosts/Settings/UserSettings.cs

@@ -54,6 +65,11 @@ public UserSettings()
            LoopbackDuplicates = false;
            AdditionalLinesPosition = HostsAdditionalLinesPosition.Top;
            Encoding = HostsEncoding.Utf8;
+            BackupHosts = true;
+            BackupPath = @"C:\Windows\system32\drivers\etc";


would that be better if change to Environment.GetEnvironmentVariable("windir")?
https://learn.microsoft.com/en-us/dotnet/api/system.environment.getenvironmentvariable?view=net-9.0&redirectedfrom=MSDN#System_Environment_GetEnvironmentVariable_System_String_

then fall back to hard-code string as needed

Nice catch!
I used the same logic we are using for the hosts file path: no issues so far, so I think a fallback isn't needed.

PowerToys/src/modules/Hosts/HostsUILib/Helpers/HostsService.cs

Line 53 in c6776d0

_hostsFilePath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.Windows), @"System32\drivers\etc\hosts");

jamrobot · 2025-03-18T01:29:22Z

src/modules/Hosts/HostsUILib/Helpers/BackupManager.cs

+                return;
+            }
+
+            if (!_fileSystem.Directory.Exists(_userSettings.BackupPath))


Suggest move to try-catch block below, in case if CreateDirectory failed (e.g.: due to permission). otherwise, unhandled exception may cause hosts crash.

jamrobot · 2025-03-18T04:20:15Z

src/settings-ui/Settings.UI.Library/HostsProperties.cs

        public HostsProperties()
        {
            ShowStartupWarning = true;
            LaunchAdministrator = true;
            LoopbackDuplicates = false;
            AdditionalLinesPosition = HostsAdditionalLinesPosition.Top;
            Encoding = HostsEncoding.Utf8;
+            BackupHosts = true;
+            BackupPath = @"C:\Windows\System32\drivers\etc";


same here, try get from EnviromentVariable?

jamrobot · 2025-03-18T04:31:38Z

src/settings-ui/Settings.UI.Library/HostsProperties.cs

        public HostsProperties()
        {
            ShowStartupWarning = true;
            LaunchAdministrator = true;
            LoopbackDuplicates = false;
            AdditionalLinesPosition = HostsAdditionalLinesPosition.Top;
            Encoding = HostsEncoding.Utf8;
+            BackupHosts = true;
+            BackupPath = @"C:\Windows\System32\drivers\etc";
+            DeleteBackupsMode = HostsDeleteBackupMode.Age;


[non-blocking-comment] it seems initialization code are dup in both Hosts (src/modules/Hosts/Hosts/Settings/UserSettings.cs) & Settings.UI.Library

is that possible to have one ?

jamrobot · 2025-03-18T04:33:21Z

src/modules/Hosts/HostsUILib/Helpers/IBackupManager.cs

+{
+    public interface IBackupManager
+    {
+        void CreateBackup(string hostsFilePath);


[non-blocking-comment] suggest change to Create, Delete.
similar naming pattern as File.Create (https://learn.microsoft.com/en-us/dotnet/api/system.io.file.create?view=net-9.0)

jamrobot · 2025-04-04T22:54:03Z

my bad, forgot to click 'Submit' to save my review comments. apology for the delay... :(

davidegiacometti · 2025-04-06T09:01:26Z

Don't worry and thanks for the review.
I have pushed the changes.

davidegiacometti · 2025-04-08T18:53:47Z

/azp run

azure-pipelines · 2025-04-08T18:53:58Z

Azure Pipelines successfully started running 1 pipeline(s).

davidegiacometti · 2025-04-17T18:45:10Z

/azp run

azure-pipelines · 2025-04-17T18:45:20Z

Azure Pipelines successfully started running 1 pipeline(s).

davidegiacometti marked this pull request as draft March 5, 2025 22:18

htcfreek reviewed Mar 6, 2025

View reviewed changes

src/settings-ui/Settings.UI/Strings/en-us/Resources.resw Outdated Show resolved Hide resolved

htcfreek reviewed Mar 6, 2025

View reviewed changes

src/settings-ui/Settings.UI/SettingsXAML/Views/HostsPage.xaml Outdated Show resolved Hide resolved

src/settings-ui/Settings.UI/SettingsXAML/Views/HostsPage.xaml Outdated Show resolved Hide resolved

src/settings-ui/Settings.UI/SettingsXAML/Views/HostsPage.xaml Outdated Show resolved Hide resolved

davidegiacometti closed this Mar 8, 2025

davidegiacometti reopened this Mar 9, 2025

htcfreek reviewed Mar 9, 2025

View reviewed changes

src/settings-ui/Settings.UI/Strings/en-us/Resources.resw Outdated Show resolved Hide resolved

src/settings-ui/Settings.UI/Strings/en-us/Resources.resw Outdated Show resolved Hide resolved

src/settings-ui/Settings.UI/Strings/en-us/Resources.resw Outdated Show resolved Hide resolved

davidegiacometti force-pushed the users/davidegiacometti/hosts-bak-settings branch 3 times, most recently from a9c52d1 to 011f108 Compare March 11, 2025 20:30

davidegiacometti mentioned this pull request Mar 11, 2025

[PowerToys] [Hosts File Editor] Backup Settings MicrosoftDocs/windows-dev-docs#5342

Open

davidegiacometti marked this pull request as ready for review March 11, 2025 21:34

davidegiacometti changed the title ~~🚧 [Hosts] Backup Settings~~ [Hosts] Backup Settings Mar 11, 2025

davidegiacometti force-pushed the users/davidegiacometti/hosts-bak-settings branch from 011f108 to 897277c Compare March 12, 2025 19:42

davidegiacometti requested a review from Copilot March 12, 2025 20:26

Copilot AI reviewed Mar 12, 2025

View reviewed changes

src/modules/Hosts/HostsUILib/Helpers/BackupManager.cs Outdated Show resolved Hide resolved

davidegiacometti requested a review from jamrobot March 12, 2025 21:31

crutkas added the Needs-Review This Pull Request awaits the review of a maintainer. label Mar 22, 2025

jamrobot reviewed Apr 4, 2025

View reviewed changes

davidegiacometti force-pushed the users/davidegiacometti/hosts-bak-settings branch from 73b0562 to 5d2f3c7 Compare April 6, 2025 09:00

yeelam-gordon added the Product-Hosts File Editor Refers to the Hosts file editor label Apr 8, 2025

jamrobot requested a review from chenmy77 April 10, 2025 06:01

jamrobot assigned jamrobot and chenmy77 Apr 10, 2025

davidegiacometti added 7 commits April 15, 2025 20:34

Add backup settings to Hosts File Editor

3437784

new UI

3bc6c63

new logic and tests

e3cd047

cleanup

6f43560

minor changes

a1f3fb8

fair comment my copilot agent :)

25ed890

addressed review feedback

1a37664

davidegiacometti force-pushed the users/davidegiacometti/hosts-bak-settings branch from 5d2f3c7 to 1a37664 Compare April 15, 2025 18:34

[Hosts] Backup Settings #37778

Are you sure you want to change the base?

[Hosts] Backup Settings #37778

Uh oh!

Conversation

davidegiacometti commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

Uh oh!

htcfreek commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidegiacometti commented Mar 6, 2025

Uh oh!

htcfreek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htcfreek commented Mar 6, 2025

Uh oh!

davidegiacometti commented Mar 6, 2025

Uh oh!

htcfreek commented Mar 6, 2025

Uh oh!

davidegiacometti commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htcfreek commented Mar 6, 2025

Uh oh!

davidegiacometti commented Mar 8, 2025

Uh oh!

htcfreek commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidegiacometti commented Mar 8, 2025

Uh oh!

htcfreek commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidegiacometti commented Mar 8, 2025

Uh oh!

htcfreek commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidegiacometti commented Mar 8, 2025

Uh oh!

davidegiacometti commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htcfreek commented Mar 8, 2025

Uh oh!

davidegiacometti commented Mar 8, 2025

Uh oh!

htcfreek commented Mar 8, 2025

Uh oh!

htcfreek commented Mar 8, 2025

Uh oh!

davidegiacometti commented Mar 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidegiacometti commented Mar 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

jamrobot Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

davidegiacometti Apr 6, 2025

davidegiacometti commented Mar 5, 2025 •

edited

Loading

htcfreek commented Mar 6, 2025 •

edited

Loading

davidegiacometti commented Mar 6, 2025 •

edited

Loading

htcfreek commented Mar 8, 2025 •

edited

Loading

htcfreek commented Mar 8, 2025 •

edited

Loading

htcfreek commented Mar 8, 2025 •

edited

Loading

davidegiacometti commented Mar 8, 2025 •

edited

Loading