gui, lib/config: Add default path for new folders (fixes #2157) #4192

imsodin · 2017-06-03T15:37:51Z

Add a default value for the path input field on new folders, by joining based the new config option defaultFolderPath and the label (or ID if none). Once the user replaces/modifies the path, further changes to the label do no longer change the path.

The current state is a minimalistic implementation, because I first wanted to get feedback before eventually going ahead.
The config is only accessible in advanced settings and the feature is only activated, if the user explicitly sets this config entry. This could be extended by:

Add this config option to "normal" settings modal.
Set a default for the default path, e.g. ~ or ~/Syncthing.

calmh · 2017-06-09T07:26:24Z

Looks reasonable to me, as such, but I'm confused by the explicit document.getElementById.dostuff instead of setting the appropriate scope variables directly? Is it to make sure form validation and stuff happens, or something like that?

imsodin · 2017-06-09T08:56:42Z

Looks reasonable to me, as such, but I'm confused by the explicit document.getElementById.dostuff instead of setting the appropriate scope variables directly? Is it to make sure form validation and stuff happens, or something like that?

It is to set the default value of the input field: I don't want to alter it any more once the user edited it manually. Setting the value attribute of the text input has this property. I am not sure whether I tried creating a dedicated variable in $scope for the default value instead of using document.getElementById - I actually quite liked this direct version without adding another variable. Should I (try to) change it?

Also while testing this feature I found out that the addition of a trailing directory name based on label/id is potentially more of a nuisance than a feature when you don't want the label to be the same as the shared directory name (which is mostly the case for me).

So maybe this should be tuned down even more to only prefill the default path?

calmh · 2017-06-09T10:02:19Z

Hmm. But the watch thing reacts to changes to currentFolder.label and does the getelement manipulation of the thing with ID folderPath which I guess maps to currentFolder.path, so it seems like a circuitous route?

AudriusButkevicius · 2017-06-09T10:05:22Z

Pls, no javascript 0.1,use jquery if you need to, but to be honest Iamnot sure I understand the issue. You should be able just to do value='{{ foibar }}'

imsodin · 2017-06-09T13:11:15Z

Hmm. But the watch thing reacts to changes to currentFolder.label and does the getelement manipulation of the thing with ID folderPath which I guess maps to currentFolder.path, so it seems like a circuitous route?

It does not map to an element in $scope, it directly sets the value attribute of the html element with id folderPath.

Pls, no javascript 0.1,use jquery if you need to, but to be honest Iamnot sure I understand the issue. You should be able just to do value='{{ foibar }}'

Like so?

-            document.getElementById('folderPath').value = path;
+            $('#folderPath').attr("value", path);

That doesn't work (nothing appearing in the input field) and doesn't give any error on the JS console.

I tried just using an additional variable "currentDefaultPath` for this:

-                    document.getElementById('folderPath').value = pathJoin($scope.config.options.defaultFolderPath, $scope.currentFolder.id);
+                    $scope.currentDefaultPath = pathJoin($scope.config.options.defaultFolderPath, $scope.currentFolder.id);

-            <input name="folderPath" ng-readonly="editingExisting" id="folderPath" class="form-control" type="text" ng-model="currentFolder.path" list="directory-list" ng-value="config.options.defaultFolderPath" required path-is-sub-dir/>
+            <input name="folderPath" ng-readonly="editingExisting" id="folderPath" class="form-control" type="text" ng-model="currentFolder.path" list="directory-list" ng-value="currentDefaultPath" required path-is-sub-dir/>

Doesn't work either, same problem: Nothing displayed in the input field, no errors and currentDefaultPath is set to the correct value. So the current version that apparently is "stylistically" unsound works, and I don't get a more sound version to work...
Alternative: Drop the label/id path suffix, just add a default path.

wweich · 2017-06-09T15:35:11Z

Values aren't set with $('#folderPath').attr("value", path); but with $('#folderPath').val(path);.

AudriusButkevicius · 2017-06-09T15:38:40Z

The template hack does not work as its both ng-model and ng-value. You can probably preset the value on the model or use plain value (non ng) with curly brace expansion.

imsodin · 2017-06-09T16:29:26Z

Values aren't set with $('#folderPath').attr("value", path); but with $('#folderPath').val(path);.

I don't want to set the value of the input field, but the value of the value attribute.

The template hack does not work as its both ng-model and ng-value. You can probably preset the value on the model or use plain value (non ng) with curly brace expansion.

The curly brace thing is what I tried initially, because I thought that was the right way to access a variable in $scope. Doesn't work either. I pushed this non-functional code, maybe there is a stupid mistake that I just can't see.

imsodin · 2017-06-13T10:43:20Z

A lucky google search pointed me to jquery's prop instead of attr function and now that works. I actually prefer it over adding another "assistance-variable" to $scope.

The principle questions asked earlier have still not been addressed:

Is it even desirable to have a default directory name appended to the default path? Or should this be another option?
Should this be exposed in the normal settings modal?
Should this be made default (e.g. setting it to "~" by default)?

AudriusButkevicius · 2017-06-13T19:40:07Z

gui/default/syncthing/core/syncthingController.js

@@ -1343,16 +1358,30 @@ angular.module('syncthing.core')
        $scope.directoryList = [];

        $scope.$watch('currentFolder.path', function (newvalue) {
-            if (newvalue && newvalue.trim().charAt(0) === '~') {
-                $scope.currentFolder.path = $scope.system.tilde + newvalue.trim().substring(1);
+            if (!(newvalue)) {


you don't need the extra braces

AudriusButkevicius · 2017-06-13T20:00:41Z

gui/default/syncthing/core/syncthingController.js

+            } else {
+                path = pathJoin(path, $scope.currentFolder.id);
+            }
+            $('#folderPath').prop("value", path);


As it's implemented now, I think it introduces a bug where if none of the fields get touched, currentFolder.path ends up being empty (yet not looking empty).

Also, it doesn't handle the case where the ID is changed by the user, yet the label is left blank. It will stick with the first ID it ever sees.

So there is four ways to solve this:

Use $dirty/$pristine checks on currentFolder.path to decide if you want to keep updating it as user tweaks the id or label. explained here

Keep updating path every time user changes the label/ID. Hopefully they settle on what it's supposed to look like before meddling with the path (annoying)

Do what you did, but somehow patch up the empty path case.

Something like this:

$scope.$watchGroup(['currentFolder.label', 'currentFolder.id'], function (newvalues, oldvalues) { if (!$scope.config.options || !$scope.config.options.defaultFolderPath || $scope.currentFolder.path) { return; } oldvalue = oldvalues[0] || oldvalues[1]; // equivalnet to .label || .id newvalue = newvalues[0] || newvalues[1]; // Label being changed, as path at old label matches current path, hence update with new ID/label. // In all other cases it's the path b if (pathJoin($scope.config.options.defaultFolderPath, oldvalue) != $scope.currentFolder.path) { $scope.currentFolder.path = pathJoin($scope.config.options.defaultFolderPath, newvalue); } }

This way you avoid the jQuery magic, and change the value only if the label is being changed and the old path is still at the correct old value.

I just did a mix of 1. and 4. and it throws errors at me, because watchGroup was introduced in AngularJS 1.3 and we are using 1.2.9. AngularJS is now at 1.6.4, is there a particular reason this was never updated?

Well I just googled and found that, but I thin .1 is probably the easiest option.

I did it with two watches instead of watchGroup. Updating angularJS up to 1.5 seemed to be fine, 1.6 broke spectacularly - I probably shouldn't touch that just as an aside.

AudriusButkevicius · 2017-06-14T10:06:42Z

gui/default/syncthing/core/syncthingController.js

-            } else {
-                path = pathJoin(path, $scope.currentFolder.id);
+            if (newValues[0]) {
+                $scope.currentFolder.path = pathJoin($scope.config.options.defaultFolderPath, newValues[0]);


This can be simplified using the or statement rather than branches.

AudriusButkevicius · 2017-06-14T10:08:23Z

Ignore my last comment, I looked at a specific commit.

AudriusButkevicius · 2017-06-14T10:08:35Z

Does this actually work the way you expect it to?

…ller)

imsodin · 2017-06-14T11:10:09Z

Does this actually work the way you expect it to?

Yes it does - there are however still the same questions open I posed earlier.
I just realised that the watch also picks up changes originating in the controller, so some of my code was redundant.

AudriusButkevicius · 2017-06-14T11:16:21Z

Answers are I think: yes, no, yes. Second is a no as they can find it in advanced config if they need it.

imsodin · 2017-06-14T11:24:28Z

Answers are I think: yes, no, yes. Second is a no as they can find it in advanced config if they need it.

I made the first question ambiguous: Yes to "it is desirable to append a dir name" or to "this should be another option"?
I am a bit skeptical about adding a default without exposing it in general options. There is so much options in Advanced that you more or less need to know the existence and name of the option in advance.

A spurious test failure was triggered, maybe related to #4065 - ignorable or a real problem? (@calmh)

AudriusButkevicius · 2017-06-14T12:45:31Z

That test failure is nothing todo with you, but if you are on a proper desktop and not a phone, open a new issue with the logs of the test for me to look at in the future.

AudriusButkevicius · 2017-06-14T12:48:25Z

Regarding your question, I think its better to show a default path than no path, even if changing it is quite cryptic as ~/Label is a reasonable default. People who really want something else can ask how to change it.

imsodin · 2017-06-14T13:53:45Z

That test failure is nothing todo with you, but if you are on a proper desktop and not a phone, open a new issue with the logs of the test for me to look at in the future.

Done: #4213

calmh · 2017-06-25T12:11:26Z

Status?

imsodin · 2017-06-25T18:31:48Z

Status?

Fine to merge I think - as long as you are ok with the "design" choice of making "~" the default path, always appending a new dir-name based on label/id and only exposing it in advanced settings.

calmh · 2017-07-20T13:16:42Z

I seem to have missed this one. For shame. :( Tested it now and it seems to work perfectly fine!

calmh · 2017-07-20T13:16:49Z

@st-review merge it!

st-review · 2017-07-20T13:16:57Z

👌 Merged as a04b923. Thanks, @imsodin!

GitHub-Pull-Request: #4192

calmh · 2017-07-20T13:18:20Z

btw other contributors, please feel free to be more active when I suck. Any two-person lgtm-handshake ought to be good enough if the change isn't clearly dangerous stuff.

GitHub-Pull-Request: syncthing#4192

gui, lib/config: Add default path for new folders (fixes syncthing#2157)

303ffda

imsodin force-pushed the defaultPath branch from 0ff71ce to 303ffda Compare June 3, 2017 15:40

imsodin changed the title ~~gui, lib/config: Add option to specify a default path for new folders (fixes 2157)~~ gui, lib/config: Add default path for new folders (fixes #2157) Jun 3, 2017

fix & polish

174e20a

add currentDefaultPath for value

4ee7014

imsodin added 2 commits June 13, 2017 12:27

Merge branch 'master' into defaultPath

7d19fe2

Use jquery to set the default path

7dadb85

AudriusButkevicius reviewed Jun 13, 2017

View reviewed changes

imsodin added 2 commits June 14, 2017 11:46

Watch both label and id, update AngularJS (1.2.9->1.3.0) and polish

181a621

Same as previous commit, without update of angularjs

9855e92

AudriusButkevicius reviewed Jun 14, 2017

View reviewed changes

Remove redundant assignment of path (watch picks up changes in contro…

0aaf3dc

…ller)

Set DefaultFolderPath to '~' by default

fc17206

imsodin added 2 commits June 14, 2017 16:31

Don't update path when editing folder

6ee777e

adjust tests

2c4f804

st-review closed this Jul 20, 2017

st-review pushed a commit that referenced this pull request Jul 20, 2017

gui, lib/config: Add default path for new folders (fixes #2157)

a04b923

GitHub-Pull-Request: #4192

imsodin mentioned this pull request Aug 10, 2017

Default path for new folders #2157

Closed

viable-hartman pushed a commit to viable-hartman/syncthing that referenced this pull request Aug 25, 2017

gui, lib/config: Add default path for new folders (fixes syncthing#2157)

51aece8

GitHub-Pull-Request: syncthing#4192

st-review added the pr-merged Legacy label used in the past for pull requests merged to the main tree label Jan 15, 2018

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

syncthing locked and limited conversation to collaborators Jul 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gui, lib/config: Add default path for new folders (fixes #2157) #4192

gui, lib/config: Add default path for new folders (fixes #2157) #4192

imsodin commented Jun 3, 2017

calmh commented Jun 9, 2017

imsodin commented Jun 9, 2017

calmh commented Jun 9, 2017

AudriusButkevicius commented Jun 9, 2017

imsodin commented Jun 9, 2017

wweich commented Jun 9, 2017

AudriusButkevicius commented Jun 9, 2017

imsodin commented Jun 9, 2017

imsodin commented Jun 13, 2017

AudriusButkevicius Jun 13, 2017 •

edited

AudriusButkevicius Jun 13, 2017 •

edited

imsodin Jun 13, 2017

AudriusButkevicius Jun 13, 2017

imsodin Jun 14, 2017

AudriusButkevicius Jun 14, 2017

AudriusButkevicius commented Jun 14, 2017

AudriusButkevicius commented Jun 14, 2017

imsodin commented Jun 14, 2017 •

edited

AudriusButkevicius commented Jun 14, 2017

imsodin commented Jun 14, 2017 •

edited

AudriusButkevicius commented Jun 14, 2017

AudriusButkevicius commented Jun 14, 2017

imsodin commented Jun 14, 2017

calmh commented Jun 25, 2017

imsodin commented Jun 25, 2017

calmh commented Jul 20, 2017

calmh commented Jul 20, 2017

st-review commented Jul 20, 2017

calmh commented Jul 20, 2017

gui, lib/config: Add default path for new folders (fixes #2157) #4192

gui, lib/config: Add default path for new folders (fixes #2157) #4192

Conversation

imsodin commented Jun 3, 2017

calmh commented Jun 9, 2017

imsodin commented Jun 9, 2017

calmh commented Jun 9, 2017

AudriusButkevicius commented Jun 9, 2017

imsodin commented Jun 9, 2017

wweich commented Jun 9, 2017

AudriusButkevicius commented Jun 9, 2017

imsodin commented Jun 9, 2017

imsodin commented Jun 13, 2017

AudriusButkevicius Jun 13, 2017 • edited

Choose a reason for hiding this comment

AudriusButkevicius Jun 13, 2017 • edited

Choose a reason for hiding this comment

imsodin Jun 13, 2017

Choose a reason for hiding this comment

AudriusButkevicius Jun 13, 2017

Choose a reason for hiding this comment

imsodin Jun 14, 2017

Choose a reason for hiding this comment

AudriusButkevicius Jun 14, 2017

Choose a reason for hiding this comment

AudriusButkevicius commented Jun 14, 2017

AudriusButkevicius commented Jun 14, 2017

imsodin commented Jun 14, 2017 • edited

AudriusButkevicius commented Jun 14, 2017

imsodin commented Jun 14, 2017 • edited

AudriusButkevicius commented Jun 14, 2017

AudriusButkevicius commented Jun 14, 2017

imsodin commented Jun 14, 2017

calmh commented Jun 25, 2017

imsodin commented Jun 25, 2017

calmh commented Jul 20, 2017

calmh commented Jul 20, 2017

st-review commented Jul 20, 2017

calmh commented Jul 20, 2017

AudriusButkevicius Jun 13, 2017 •

edited

AudriusButkevicius Jun 13, 2017 •

edited

imsodin commented Jun 14, 2017 •

edited

imsodin commented Jun 14, 2017 •

edited