Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gui, lib/versioner: Allow to configure staggered versioning intervals (fixes #8352) #9094

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

tomasz1986
Copy link
Contributor

@tomasz1986 tomasz1986 commented Sep 10, 2023

gui, lib/versioner: Allow to configure staggered versioning intervals (fixes #8352)

Currently, there are four intervals used in the staggered versioning,
with all of them but the Maximum Age being hard-coded. With this change,
the user is able to configure all intervals to their liking. Due to the
complexity of the configuration, this option is intended to be used by
advanced users only.

In addition to the above, also:

  1. Add yet another interval for more fine-grained configuration
    possibilities. By default, the new interval activates after one year,
    keeping one version per month. However, because the default Maximum
    Age is also set to one year, this fifth interval is actually not
    utilised unless the user increases the Maximum Age to a higher value.

  2. Fix a small bug in the Staggered Versioning test where the default
    time period for the third interval was supposed to be set to 30 days,
    but in reality it was set to just 7 days.

Signed-off-by: Tomasz Wilczyński twilczynski@naver.com

Screenshots

image
image

…fixes syncthing#8352)

Currently, there are four intervals used in the staggered versioning,
with all of them but the Maximum Age being hard-coded. With this change,
the user is able to configure all intervals to their liking. Due to the
complexity of the configuration, this option is intended to be used by
advanced users only.

In addition to the above, also:

1. Add yet another interval for more fine-grained configuration
   possibilities. By default, the new interval activates after one year,
   keeping one version per month. However, because the default Maximum
   Age is also set to one year, this fifth interval is actually not
   utilised unless the user increases the Maximum Age to a higher value.

2. Fix a small bug in the Staggered Versioning test where the default
   time period for the third interval was supposed to be set to 30 days,
   but in reality it was set to just 7 days.

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
@tomasz1986
Copy link
Contributor Author

I will be grateful for any tips on how to simplify this code 😅.

@@ -115,19 +115,89 @@
<span translate ng-if="folderEditor.simpleKeep.$error.min && folderEditor.simpleKeep.$dirty">You must keep at least one version.</span>
</p>
</div>
<div class="form-group" ng-if="currentFolder._guiVersioning.selector=='staggered'" ng-class="{'has-error': folderEditor.staggeredMaxAge.$invalid && folderEditor.staggeredMaxAge.$dirty}">
<p class="help-block"><span translate>Files are moved to date stamped versions in a .stversions directory when replaced or deleted by Syncthing.</span> <span translate>Versions are automatically deleted if they are older than the maximum age or exceed the number of files allowed in an interval.</span></p>
<p translate class="help-block">The following intervals are used: for the first hour a version is kept every 30 seconds, for the first day a version is kept every hour, for the first 30 days a version is kept every day, until the maximum age a version is kept every week.</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to break this very long translation, so unfortunately parts of it will have to be translated again, but this time I've also tried to split this into separate strings, so that it should be easier to deal with from now on if there are any small changes required.

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
@calmh
Copy link
Member

calmh commented Sep 11, 2023

Didn't look at any code yet, but I can say that exposing config values in the range of millions of seconds in the non-advanced GUI certainly doesn't seem ideal. Humans generally use different units at those time scales.

@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Sep 11, 2023

Yeah, I know about that. I originally just tried using time scales that are mentioned in the code comments (day/hour/week), however that didn't allow for such fine-grained control as the current iteration (which the user in the enhancement request wanted).

I think there could be two solutions to this:

  • Leave the first interval at seconds, change the second one to hours, and then the rest to days. The Maximum Age, i.e. the last period, is already set using days, so I don't think it makes sense to use any larger time units than days.

or

  • make it so that if you click on the time unit label, it cycles through seconds/hours/days/weeks/months/years, etc. which would allow to set the larger intervals much easier while still allowing for the current fine-grained control.

The second approach would probably be ideal and much more flexible, but I'm not sure if I can pull this off in a reasonable amount of time without extreme code complexity, so for now I'll probably just try out the first one and see how it works.

Edit:

I'm in the process of re-writing this. The final code is going to be much different than the current iteration, so if anyone is willing to look at this PR, please wait for the next version and don't waste your time on what is available now 😅. I will make it clear when the PR is ready for review.

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
@tomasz1986 tomasz1986 marked this pull request as draft September 11, 2023 21:35
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
@uok
Copy link
Contributor

uok commented Sep 15, 2023

The information about default intervals is helpful!
My suggestion is to make the interval configuration available for advanced users by clicking a link "configure intervals".
Maybe make input it in the style of a sentence, e.g.: "for the first ___ minutes keep a version for every ___ seconds" (___ = input box)
In the folder overview the icon/tooltips are only needed if settings differ from default values.
Keep in mind that only a small percentage of users is using staggered versioning!

@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Sep 15, 2023

The information about default intervals is helpful!

The information is already there right now! It's just because I added one more interval, I had to modify the sentence, and I though that the whole thing would be easier to read and understand when split into a bullet point list.

My suggestion is to make the interval configuration available for advanced users by clicking a link "configure intervals".
Maybe make input it in the style of a sentence, e.g.: "for the first ___ minutes keep a version for every ___ seconds" (___ = input box)

Do you think it would be better to have it displayed in a separate modal? It's definitely possible to do, but this part is only visible when you're using Staggered Versioning already, so that's why I just added it to the main File Versioning configuration window together with the other, already existing options. Of course, it's not visible when using other types of versioning (or no versioning at all).

Using human language would be nice, but this kind of sentence is a nightmare when it comes to translation. I don't think it's actually possible (e.g. because "2 minutes" and "5 minutes" use different noun forms depending on the number in languages like Polish).

In the folder overview the icon/tooltips are only needed if settings differ from default values.

This has recently been changed in #8987. We now show all values regardless of whether they are default or not.

@calmh
Copy link
Member

calmh commented Sep 25, 2023

Maybe we could have a time picker of some kind.

Unknown

@tomasz1986 tomasz1986 added the slow-pr Pull requests that are worked on infrequently label Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
slow-pr Pull requests that are worked on infrequently
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Staggered File Versioning - intervals as parameters
3 participants