🚧 [Hosts] Backup Settings

[davidegiacometti]

Summary of the Pull Request

PR Checklist

• Closes: Hosts file editor not making backups and losing my file #37666
• Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
• 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
• Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

[htcfreek]

Suggested change

	<value>Backups will never will be kept indefinitely with the current configuration</value>
	<value>Backups never will be kept indefinitely with the current configuration</value>

[davidegiacometti]

Thanks for catching this (haven't reviewed the PR yet 😄).
What I wanted here is Backups will be kept indefinitely with the current configuration.

[htcfreek]

The UI is confusing: If I keep my backups 0 days, then I have no backups. 🤔

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

some ui nits

[htcfreek]

is enabled should be bind against both: backup hosts and automatic deletion

[htcfreek]

is enabled should be bind against both: backup hosts and automatic deletion

[htcfreek]

For a nicer UI: Can we move it as first element in the expander.

[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 #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]

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.