gui, lib/model: Disable periodic cleanup when versions are kept forever (fixes #8350) #8994
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…er (fixes syncthing#8350) Currently, the cleanup is set to run on a periodic timer regardless of the days to keep versions. Because of this, even if versions are set to be kept forever, the cleanup still runs on the timer. Furthermore, the cleanup also runs even when using external versioning in spite of it doing nothing for that versioning type. This changes the behaviour as follows. 1. Cleanup is disabled when using external versioning. 2. Cleanup only runs when the days to keep versions are set to a value higher than "0". 3. When using Trash Can, Simple or Staggered versioning with the value above set to "0", the cleanup being "Disabled" is displayed in the folder information in the GUI even if the cleanup value is at its default. 4. In addition to the above, the cleanup input field is automatically disabled with a warning message informing the user about it being disabled when the number of days is set to "0". Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
calmh
reviewed
Jul 21, 2023
| @@ -161,7 +171,7 @@ func (f *folder) Serve(ctx context.Context) error { | |||
|
|
|||
| // If we're configured to not do version cleanup, or we don't have a | |||
| // versioner, cancel and drain that timer now. | |||
| if f.versionCleanupInterval == 0 || f.versioner == nil { | |||
| if f.versionCleanupInterval == 0 || ((f.versionType == "trashcan" || f.versionType == "simple") && f.versionCleanoutDays == 0) || (f.versionType == "staggered" && f.versionMaxAge == 0) || f.versionType == "external" || f.versioner == nil { | |||
There was a problem hiding this comment.
This stuff could in fact happen in the constructor instead, then we don't need to have instance variables for all this stuff and it would keep things more contained, I think... We could also break up and comment the very long if clause a little bit.
acolomb
requested changes
Jul 21, 2023
| <span ng-if="folder.versioning.cleanupIntervalS != versioningDefaults.cleanupIntervalS" tooltip data-original-title="{{'Cleanup Interval' | translate}}"> | ||
|  <span class="fa fa-recycle"></span> <span ng-if="folder.versioning.cleanupIntervalS == 0" translate>Disabled</span><span ng-if="folder.versioning.cleanupIntervalS > 0">{{folder.versioning.cleanupIntervalS | duration}}</span> | ||
| <span ng-if="folder.versioning.cleanupIntervalS != versioningDefaults.cleanupIntervalS || (folder.versioning.cleanupIntervalS == versioningDefaults.cleanupIntervalS && (((folder.versioning.type == 'trashcan' || folder.versioning.type == 'simple') && folder.versioning.params.cleanoutDays == 0) || (folder.versioning.type == 'staggered' && folder.versioning.params.maxAge == 0)))" tooltip data-original-title="{{'Cleanup Interval' | translate}}"> | ||
|  <span class="fa fa-recycle"></span> <span ng-if="folder.versioning.cleanupIntervalS == 0 || (((folder.versioning.type == 'trashcan' || folder.versioning.type == 'simple') && folder.versioning.params.cleanoutDays == 0) || (folder.versioning.type == 'staggered' && folder.versioning.params.maxAge == 0))" translate>Disabled</span><span ng-if="folder.versioning.cleanupIntervalS > 0 && (((folder.versioning.type == 'trashcan' || folder.versioning.type == 'simple') && folder.versioning.params.cleanoutDays != 0) || (folder.versioning.type == 'staggered' && folder.versioning.params.maxAge != 0))">{{folder.versioning.cleanupIntervalS | duration}}</span> |
There was a problem hiding this comment.
This is way too much controller logic coded into the GUI presentation for my taste. Please if there is no easier way to determine all these conditions, then at least move it to the controller JS code, encapsulated as a function. Stuff like this tends to get forgotten when the underlying logic changes (such as when a new versioning scheme gets introduced). So more visibility and ideally close to the versioning code itself is better for maintenance.
| <div class="input-group-addon" translate>seconds</div> | ||
| </div> | ||
| <p class="help-block"> | ||
| <span translate ng-if="folderEditor.cleanupIntervalS.$valid || folderEditor.cleanupIntervalS.$pristine"class="help-block">The interval, in seconds, for running cleanup in the versions directory. Zero to disable periodic cleaning.</span> | ||
| <span translate ng-if="folderEditor.cleanupIntervalS.$error.required && folderEditor.cleanupIntervalS.$dirty">The cleanup interval cannot be blank.</span> | ||
| <span translate ng-if="folderEditor.cleanupIntervalS.$error.min && folderEditor.cleanupIntervalS.$dirty">The interval must be a positive number of seconds.</span> | ||
| <span class="text-warning" ng-if="(((currentFolder._guiVersioning.selector == 'trashcan' || currentFolder._guiVersioning.selector == 'simple') && currentFolder._guiVersioning.trashcanClean == 0) || (currentFolder._guiVersioning.selector == 'staggered' && currentFolder._guiVersioning.staggeredMaxAge == 0))"> |
There was a problem hiding this comment.
Seems like this logic is repeated in several places. One more reason to factor it out into a controller method.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
gui, lib/model: Disable periodic cleanup when versions are kept forever (fixes #8350)
Currently, the cleanup is set to run on a periodic timer regardless of
the days to keep versions. Because of this, even if versions are set to
be kept forever, the cleanup still runs on the timer. Furthermore, the
cleanup also runs even when using external versioning in spite of it
doing nothing for that versioning type.
This changes the behaviour as follows.
higher than "0".
above set to "0", the cleanup being "Disabled" is displayed in the
folder information in the GUI even if the cleanup value is at its
default.
disabled with a warning message informing the user about it being
disabled when the number of days is set to "0".
Screenshots