[Hosts] Backup Settings

[davidegiacometti]

Summary of the Pull Request

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

PR Checklist

• [x] Closes: #37666
• [ ] Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
• [x] Tests: Added/updated and all pass
• [ ] Localization: All end user facing strings can be localized
• [ ] Dev docs: Added/updated
• [ ] New binaries: Added on the required places
  • [ ] JSON for signing for new binaries
  • [ ] WXS for installer for new binaries and localization folder
  • [ ] YML for CI pipeline for new test projects
  • [ ] YML for signed pipeline
• [x] Documentation updated: If checked, please file a pull request on our docs repo and link it here: https://github.com/MicrosoftDocs/windows-dev-docs/pull/5342

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 🚀

[htcfreek]

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

[davidegiacometti]

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]

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]

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 https://github.com/microsoft/PowerToys/issues/37666#issuecomment-2689795329 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]

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]

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]

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]

@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]

@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]

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]

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]

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]

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]

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]

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.

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]

@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]

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]

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]

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]

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

[davidegiacometti]

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.

[jamrobot]

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

[davidegiacometti]

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

[davidegiacometti]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[davidegiacometti]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[niels9001]

@davidegiacometti we've let this PR hanging for too long - apologies for that! Would you mind looking at the conflicts, so we can get this reviewed?

[davidegiacometti]

Hi @niels9001 It's been a while, but thanks for the update on this! 😃
I have aligned the branch with latest main and fixed the conflicts.