all: Add configurable defaults (fixes #4224, fixes #6086) #7131

imsodin · 2020-11-19T18:03:10Z

The PR is still WIP because of the to-dos below, but it is usable as it is right now.

The branch lives on top of #7127 (first three commits at the time of writing) - I would suggest looking at that first, however if you'd rather have one huge diff, it's available here :)

The defaults in proto are represented as normal FolderConfiguration/DeviceConfiguration messages. I added a new ext.nodefault proto extension/tag to mark fields which are not configurable (like the id). Those are used to ensure that they are at their zero value when committing a config change. Using the same type allows using it directly as a template for new folders/devices and means there's no duplication.

The defaults can be obtained and set through the rest api on rest/config/defaults/folder resp. .../device and that endpoint is used instead of the hardcoded values in the GUI (fixes #6068).

Changed defaults

If the defaults in the GUI did not match those in the config, I chose the GUI ones as those are the ones that were really in effect:

Folder:

max conflicts: -1 -> 10
min disk free: nothing/unlimited -> 1%

To-do

GUI to modify defaults. Plan is to reuse existing folder and device modals. (Required to really fix Editable default values for folders, devices #4224).
Port gui to untrusted.

optional:

Somehow handle versioning defaults. Currently you can set default versioning for new folders including parameters, but not default parameters for the different versioning types. Not sure whether that's really necessary and the GUI versioning code is a bit messy/verbose, so haven't looked into that yet.

Testing

I extended the test checking defaults and did some manual fiddling around with config and UI, which looked good (but was likely far from comprehensive).

tomasz1986 · 2020-11-19T23:51:27Z

Does this also fix #6748 and #7033?

Folder:

max conflicts: -1 -> 10

min disk free: nothing/unlimited -> 1%

What about maxConcurrentWrites (supposed to be 2, but the GUI sets it to 0)?

imsodin · 2020-11-20T08:06:01Z

That's not directly affected by this PR: The GUI didn't set it at all, like many other fields, which results in zero values. Handling that for maxConcurrentWrites has already been fixed in #7069

…#6068)

imsodin · 2020-12-16T16:23:06Z

I adjusted the folder/device edit modals and accompanying JS to handle editing defaults too. Basically hid items for which defaults don't make sense (ids, remove buttons, ...).

They are accessible through the settings modal: Clicking on a button opens another modal to edit the settings, and dismissing that modal drops you back to the settings modal.

This is ready for review. Only thing left is mirroring the changes in the "untrusted GUI code".

tomasz1986 · 2020-12-17T00:25:34Z

They are accessible through the settings modal: Clicking on a button opens another modal to edit the settings, and dismissing that modal drops you back to the settings modal.

I have been trying to test, but nothing happens when trying to click any of those buttons. I have compiled from https://github.com/imsodin/syncthing/tree/all/defaultConfig and also checked a few different browsers (Chromium, IE, Palemoon), but there is no difference. No reaction at all, and no JS errors either.

imsodin · 2020-12-17T11:03:54Z

Works fine for me on firefox and chromium (linux) - no idea what the problem could be. There's no new source file I have forgotten to check in, nor do I have a "gui" dir in my config (you probably don't either, as you're seeing the new buttons). To debug you could inspect the buttons using developer tools, they should have attributes like ng-click=editFolderDefaults(). If that's there, find that function (or it's absence) in syncthingController.js and add a breakpoint to see what happens (or doesn't).

The gui changes are now also applied to the untrusted files, i.e. this is fully ready now. Except of course we need to find out why it doesn't work for @tomasz1986 😬

tomasz1986 · 2020-12-17T11:49:53Z

I have recompiled with the newest commits, and it is working now! I am not sure what the problem was about.

However, I have tried to fiddle around with the actual settings and something seems to be wrong…

I set the folder defaults as follows.

This is what I get when trying to create a new folder.

Also, just a side question, but I can see the current device's ID included under the defaults element in config.xml. Is there any reason or requirement for it to be there?

imsodin · 2020-12-17T15:56:19Z

Thanks a lot @tomasz1986 !

I removed Options.DefaultFolderPath, that is now Defaults.Folder.Path. In the usage reporting I kept the variable, though it's a bit of an outlier now. Respectively we might add a new metric that checks for defaultFolder := config.FolderConfiguration; util.SetDefaults(&cfg); reflect.DeepEqual(Defaults.Folder, defaultFolder). However such a blanket check doesn't seem particularly insightful to me.

Also, just a side question, but I can see the current device's ID included under the defaults element in config.xml. Is there any reason or requirement for it to be there?

The local directory always is part of every folder, so to me it makes sense that it's also part of the default folder. All the gui and go code would add the local device anyway immediately if it was missing (i.e. there would have to be special code just to keep it out of the "default folder").

tomasz1986 · 2020-12-18T01:15:17Z

I am not sure whether this is a complete version, but I have tested the current state anyway.

Right now, the File Versioning and Advanced settings seem to be applied fine. The folder ID is again placed in the proper field. The Folder Label is still empty though.

There is also this strange glitch.

Set the default folder path to .\abc.
Start adding new folder, but cancel before saving.
The default folder path has now changed to .\abc\abc

imsodin · 2020-12-18T09:02:21Z

I am not sure whether this is a complete version, but I have tested the current state anyway.

Except for the glitches you find - it is :)

There is also this strange glitch.

I cannot reproduce. Steps I took:

Open folder defaults, set default folder path to \.abc, save.
Click add folder. The displayed path is \.abc/*folder-id*.
Click close.

Result: The path in both the add folder dialog and the default folder config dialog is still \.abc/*folder-id* resp. \.abc

Probably unrelated: .\abc is strange to me. Is \ a path separator here? And I do think relative paths work, as in "something happens" (and assume they are relative to the dir from which syncthing was started?), but it's not "on purpose", as in the desired behaviour is not defined and thus might change -> I would not recommend using it.

tomasz1986 · 2020-12-18T09:35:19Z

It must be .\abc (or possibly ./abc, but not \.abc 😁), and yes, the \ is just a separator here, relative to the path, where Syncthing has been started. In most cases, I do use full paths, but using relative paths is handy for portable installations (e.g. on a USB stick).

Regardless of the relative paths, I think that I have found the culprit though.

Set the default folder label to abc
Set the default folder path to .

Try creating a new folder, but do not save it.
The default folder path has changed to .\abc.

The label is basically added to the path. Is this how it should behave? If yes, then why do we need both the label and the path?

I have also found another problem. When trying to add a folder, nothing is displayed under Sharing, even though I have multiple devices connected.

tomasz1986 · 2020-12-19T00:42:51Z

I have tested the commit in its current state. The folder path and folder sharing issues are gone 👏.

The only remaining problem that I can see at the moment is that default folder label does not seem to work at all. When trying to add a folder, the label is just empty, regardless of the default one.

tomasz1986 · 2020-12-19T15:30:47Z

I have tested the newest iteration, and everything seems to work as intended 👍.

I have just two questions/comments.

If you compare the folder defaults screen and the add folder screen, then there is one obvious difference. That is, there is no "Ignore patterns" tab in the former. I am not asking you to do it, but would there be any technical reason not to include default ignore patterns in config.xml too?
Update lucas-clemente/quic-go sum #6068 seems to be a closed PR about a bug in QUIC. Is it really related/fixed by this commit?

imsodin · 2020-12-19T16:55:53Z

That mostly just feels out of scope for this PR: It takes existing config elements to store defaults. Ignores are not part of config. That could be made part of config, or as a separate file. Then there's the question whether we want defaults as in a template, or additionally "common" ignore patterns, as in patterns that act as if they were included. Basically a soon as you introduce ignores, it gets complicated/bikesheddy. Lets get this done and then we'll see what comes after ;)
That's a typo indeed, should have been Default Folder uses different config than user-created folders #6086. And Folder defaults are not applied consistently on config migrations / new folders #6748 is at least related too.

tomasz1986 · 2020-12-19T17:53:21Z

Yeah, I was just thinking about what would be nice to have, regardless of the technical complexity. Especially since if you do integrate ignores like that, then then the question comes up, why do we need .stignore at all? Anyhow, as someone with tons of folders, I am definitely looking forward to having this PR implemented, no matter if without ignores.
Please add Default Folder uses different config than user-created folders #7033 to the mix too 🙂. It is already partially fixed (regarding maxConcurrentWrites), and this commit fixes the rest.

AudriusButkevicius · 2020-12-24T15:19:01Z

gui/default/syncthing/core/syncthingController.js

+            $scope.editingDefaults = false;
+            return $http.get(urlbase + '/config/defaults/folder').then(function(p) {
+                $scope.currentFolder = p.data;
+                for (var k in $scope.versioningDefaults) {


Why are these not stored in the folder config?

Two parts: One was a bug in the current PR, the other is a conscious omission:
Bug: The default versioning type including params is stored in config.defaults.folder.versioning, however wasn't applied when adding a new folder. That's now fixed, along with moving the gui-only parameters into currentFolder._guiVersioning.
Omission: Versioning parameters are currently "free-form" (map[string]string), which prevents using ext.default in proto. The consistent thing to do would be creating proto types for all of these and adding a Config.Defaults.VersioningParams with a new map-like struct (type -> params). That just feels like a lot of work for little gain (defaults for something that's off by default).

tomasz1986 · 2021-01-09T04:46:37Z

Do you think that it would be safe to use this commit as it is in production right now?

I have been doing a lot of folder reorganisation and such recently, and their configuration is a bit of a struggle. Do you expect any major changes in the future commits that could break the current state (e.g. changes to the config.xml structure, etc.)?

calmh

Awesome

acolomb · 2021-02-05T23:29:26Z

~~Was trying to check this out for local inspection. @imsodin I can't find this branch in your fork anymore?~~

Sorry for the noise, GitHub just didn't refresh the page...

calmh · 2021-02-08T07:30:09Z

The set of configurable options in the default settings dialog is somewhat limited; should these new pseudo-device/folders perhaps show up in the advanced config editor as well?

Utilize the newly introduced "folder default configuration" [1] feature to preset the ignorePerms option to true. This will not touch any existing folders, but set the option when configuring a new folder to be synced. Specifically on DSM, the ignorePerms option should definitely be used with every Syncthing folder. Otherwise, it will possibly try to override the UNIX permissions on synced files, which will reset the ACL-based permissions on the content. Possibly making them unusable for the rest of the DSM services. Note that this also only applies for new installations, because config.xml is (and must be) kept during updates. [1]: syncthing/syncthing#7131

@acolomb

…t. (#4527) * syncthing: Incorporate upstream release v1.14.0. Only package version and hashes updated. Built successfully for all-supported. Note that the previous package version should have already self-updated an installed Syncthing binary when the service was running long enough. * syncthing: Ignore permissions by default for new shared folders. Utilize the newly introduced "folder default configuration" [1] feature to preset the ignorePerms option to true. This will not touch any existing folders, but set the option when configuring a new folder to be synced. Specifically on DSM, the ignorePerms option should definitely be used with every Syncthing folder. Otherwise, it will possibly try to override the UNIX permissions on synced files, which will reset the ACL-based permissions on the content. Possibly making them unusable for the rest of the DSM services. Note that this also only applies for new installations, because config.xml is (and must be) kept during updates. [1]: syncthing/syncthing#7131 * syncthing: Do not cache .stignore file patterns by default. Change the default for the cacheIgnoredFiles option to false. From the Syncthing documentation [1]: Whether to cache the results of ignore pattern evaluation. Performance at the price of memory. Defaults to false as the cost for evaluating ignores is usually not significant. The DiskStations are usually rather tight on RAM, so deviating from Syncthing's default here is probably a bad idea for most users. It only matters when many (or very complex) ignore patterns are used in a folder, so usually for power users. Those will certainly be able to change the option manually. Note that this also only applies for new installations, because config.xml is (and must be) kept during updates. [1]: https://docs.syncthing.net/users/config.html * syncthing: Remove / update obsolete default options. Drop the symlinksEnabled and weakHashSelectionMethod entries from the default config file template. See [1] and [2]. Switch from the minHomeDiskFreePct option to the new syntax minHomeDiskFree unit="%". Note that this also only applies for new installations, because config.xml is (and must be) kept during updates. Syncthing migrates old configuration formats on its own. [1]: syncthing/docs@392dbd0b [2]: syncthing/syncthing#4767 * syncthing: Add myself (@acolomb) as maintainer. * Syncthing: Drop busybox and DSM 5 support Required to prepare for DSM 7.0 support * Syncthing: fixes * Framework: log the actual command after service_prestart() * syncthing: Incorporate upstream release v1.15.1. Only package version and hashes updated. Built successfully for all-supported. Note that the previous package version should have already self-updated an installed Syncthing binary when the service was running long enough. Co-authored-by: Sebastian Schmidt <publicarray@users.noreply.github.com>

all: Add configurable defaults (fixes syncthing#4224, fixes syncthing…

d92f8a8

…#6068)

imsodin force-pushed the all/defaultConfig branch from a161746 to d92f8a8 Compare November 20, 2020 15:03

imsodin added 2 commits December 16, 2020 13:31

Merge branch 'main' into all/defaultConfig

4c16188

add gui controls for folder/device defaults

d4ced7e

imsodin added 2 commits December 17, 2020 11:18

folder path fixes

35359c9

cover untrusted gui

676fcaf

imsodin marked this pull request as ready for review December 17, 2020 11:04

refactor & semicolons

5c5682e

imsodin added 6 commits December 17, 2020 15:49

more refactor and fix

f8c1d17

remove Options.DefaultFolderPath

5ece7d2

don't warn about being a parent for default folder path

34da69c

fix

0b46988

setup versioning when adding folders

88b470e

fixes

56471f0

imsodin added 2 commits December 17, 2020 20:44

don't legacy

ccc3f17

Merge branch 'main' into all/defaultConfig

53c2613

imsodin force-pushed the all/defaultConfig branch from 62c2d39 to 53c2613 Compare December 17, 2020 22:53

imsodin added 2 commits December 18, 2020 20:07

Don't add id/label to path in defaults

09ea5b7

fix sharing on add folder

413f754

imsodin changed the title ~~all: Add configurable defaults (fixes #4224, fixes #6068)~~ all: Add configurable defaults (fixes #4224, fixes #6086) Dec 19, 2020

Merge branch 'main' into all/defaultConfig

c97b779

AudriusButkevicius reviewed Dec 24, 2020

View reviewed changes

imsodin added 3 commits December 26, 2020 10:49

fix and refactor versioning params

cbb7fc0

fix race

4e27735

Merge branch 'main' into all/defaultConfig

7b3d63d

imsodin added 2 commits January 16, 2021 20:45

Merge branch 'main' into all/defaultConfig

4a45751

fix versioning again

0d42535

imsodin mentioned this pull request Feb 4, 2021

all: Automatic/disabled folder-config when receive-encrypted #7327

Merged

calmh approved these changes Feb 4, 2021

View reviewed changes

calmh merged commit ffc14a7 into syncthing:main Feb 4, 2021

calmh added this to the v1.14.0 milestone Feb 4, 2021

imsodin deleted the all/defaultConfig branch February 4, 2021 21:29

christianprescott mentioned this pull request Feb 7, 2021

gui: fix device config not saving after changes #7339

Merged

imsodin added a commit to imsodin/syncthing that referenced this pull request Feb 8, 2021

gui: Add defaults config to advanced menu (ref syncthing#7131)

cb68697

calmh pushed a commit that referenced this pull request Feb 8, 2021

gui: Add defaults config to advanced menu (ref #7131) (#7343)

e95d005

imsodin mentioned this pull request Feb 13, 2021

gui: Fix setting external versioning command (fixes #7361) #7362

Merged

acolomb mentioned this pull request Mar 28, 2021

syncthing: Upgrade to v1.15.1 and ignore folder permissions by default. SynoCommunity/spksrc#4527

Merged

3 tasks

bt90 mentioned this pull request Apr 17, 2021

Document folder and device defaults syncthing/docs#634

Closed

st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Feb 5, 2022

syncthing locked and limited conversation to collaborators Feb 5, 2022

Uh oh!

all: Add configurable defaults (fixes #4224, fixes #6086) #7131

all: Add configurable defaults (fixes #4224, fixes #6086) #7131

Uh oh!

Conversation

imsodin commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changed defaults

To-do

Testing

Uh oh!

tomasz1986 commented Nov 19, 2020

Uh oh!

imsodin commented Nov 20, 2020

Uh oh!

imsodin commented Dec 16, 2020

Uh oh!

tomasz1986 commented Dec 17, 2020

Uh oh!

imsodin commented Dec 17, 2020

Uh oh!

tomasz1986 commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imsodin commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomasz1986 commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imsodin commented Dec 18, 2020

Uh oh!

tomasz1986 commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomasz1986 commented Dec 19, 2020

Uh oh!

tomasz1986 commented Dec 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imsodin commented Dec 19, 2020

Uh oh!

tomasz1986 commented Dec 19, 2020

Uh oh!

AudriusButkevicius Dec 24, 2020

Choose a reason for hiding this comment

Uh oh!

imsodin Dec 26, 2020

Choose a reason for hiding this comment

Uh oh!

tomasz1986 commented Jan 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calmh left a comment

Choose a reason for hiding this comment

Uh oh!

acolomb commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calmh commented Feb 8, 2021

Uh oh!

Uh oh!

imsodin commented Nov 19, 2020 •

edited

Loading

tomasz1986 commented Dec 17, 2020 •

edited

Loading

imsodin commented Dec 17, 2020 •

edited

Loading

tomasz1986 commented Dec 18, 2020 •

edited

Loading

tomasz1986 commented Dec 18, 2020 •

edited

Loading

tomasz1986 commented Dec 19, 2020 •

edited

Loading

tomasz1986 commented Jan 9, 2021 •

edited

Loading

acolomb commented Feb 5, 2021 •

edited

Loading