gui: Add dropdown menu to choose discovered devices to device modal (fixes #4157) #4186

imsodin · 2017-05-31T21:32:14Z

Purpose

Testing

Manual poking around.

Screenshots

http://i.imgur.com/8TjO0Q8.png
http://i.imgur.com/iEIPV0w.png

Implementation

I needed the isEmptyObject function within the modal view. I don't know what the "correct" way of doing this in JS/Angular is, I just assigned it to $scope.isEmptyObject.

…ixes syncthing#4157)

AudriusButkevicius · 2017-05-31T21:42:34Z

gui/default/syncthing/core/syncthingController.js

@@ -1807,4 +1811,6 @@ angular.module('syncthing.core')
                window.localStorage["metricRates"] = $scope.metricRates;
            } catch (exception) { }
        }
+
+        $scope.isEmptyObject = isEmptyObject;


You don't need this, empty object is already false'ish.

AudriusButkevicius · 2017-05-31T21:50:19Z

gui/default/syncthing/core/syncthingController.js

@@ -1226,6 +1227,9 @@ angular.module('syncthing.core')

        $scope.saveDevice = function () {
            $('#editDevice').modal('hide');
+            if ($scope.currentDevice.deviceID === '') {
+                $scope.currentDevice.deviceID = $scope.manualDeviceID;
+            }


I wonder if something like this works instead, removing the need for a scope variable

<select ng-if="!editingExisting && discovery" class="form-control" ng-model="currentDevice.deviceID"> <option ng-repeat="(id, data) in discovery" value="{{id}}">{{id}}</option> <option ng-value='currentDevice.deviceID' translate>Enter manually below</option> </select>

What would you base the ng-if of the text input field on then?

I am not sure why you need that to be honest. What's the exact problem?

I don't want to display a text input field unless "Enter manually below" is selected. its ugly(er) and then the user could theoretically both choose an ID from the list and enter one below.

AudriusButkevicius · 2017-05-31T21:51:51Z

gui/default/syncthing/device/editDeviceModalView.html

        <div ng-if="editingExisting" class="well well-sm text-monospace" select-on-click>{{currentDevice.deviceID}}</div>
        <p class="help-block">
-          <span translate ng-if="deviceEditor.deviceID.$valid || deviceEditor.deviceID.$pristine">The device ID to enter here can be found in the "Actions > Show ID" dialog on the other device. Spaces and dashes are optional (ignored).</span>
+          <span translate ng-if="!currentDevice.deviceID && !isEmptyObject(discovery) && deviceEditor.deviceID.$pristine">Choose a discovered device ID or enter one manually.</span>
+          <span translate ng-if="!currentDevice.deviceID && deviceEditor.deviceID.$pristine">The device ID can be found in the "Actions > Show ID" dialog on the other device. Spaces and dashes are optional (ignored).</span>


Why removal of the $valid and the false'ish check?

I don't see the point of telling the user where to find the device ID when a valid one has already been entered or selected -> removal of $valid and checking whether an ID has been selected in from the list.

AudriusButkevicius · 2017-05-31T21:58:32Z

So this is a nice feature an all, but UX/UI wise it's a car crash.

I think it might be better to make some sort of expandable thing "Devices nearby" like advanced settings in folder editor that lists the devices.

We should also improve local discovery to put friendly name in the local discovery packet and display that if available along the ID.

Another alternative would be CSS HAX such as https://stackoverflow.com/a/31734324
We should check how that looks on the mobile though.

imsodin · 2017-05-31T22:23:58Z

So this is a nice feature an all, but UX/UI wise it's a car crash.

Agreed, could be much nicer, I just wanted a compact and quick solution because I really think that feature helps a lot for usability and I really totally missed it before. I would be happy if someone with actual frontend skills did something nice.

AudriusButkevicius · 2017-05-31T22:35:15Z

if someone with actual frontend skills did something nice.

Don't we all? like to the whole UI perhaps maybe?

calmh · 2017-06-01T05:47:26Z

How about something more HTMLy? Maybe not the most beautiful, but quite clear?

        <p class="help-block" ng-if="!editingExisting && discovery">
          <span translate>You can also select one of these nearby devices:</span>
          <ul>
            <li ng-repeat="(id, data) in discovery"><a ng-click="currentDevice.deviceID = id">{{id}}</a>
            <span class="text-muted">{{data.addresses.join(", ")}}</span></li>
          </ul>
        </p>

AudriusButkevicius · 2017-06-01T06:44:43Z

I wonder how that looks on the fossdem wifi

calmh · 2017-06-01T06:53:28Z

Similar to the wifi network dropdown in the middle of a busy city, if we are successful. I don't think it matters.

AudriusButkevicius · 2017-06-01T06:55:00Z

Fine. I still think we should ramp it up a notch by adding friendly name to local discovery packets.

uok · 2017-06-01T07:08:22Z

My vote is against friendly names unless it is clear to the user that friendly name is broadcast. "local discovery" means "in the same network", e.g. public wifi, hotel wifi - correct?

AudriusButkevicius · 2017-06-01T07:32:07Z

Usually these sort of networks block multicast. Furthermore, on Windows, as you connect to a network it asks you what type of network it is, if you select public (like you should) local discovery would be blocked.

I also think you can probe the hostname of a machine on the local network without needing syncthing anyway, so omitting the name is more of a nuisance oppose to a privacy measure. I recall doing that using using a piece of software.

uok · 2017-06-01T07:40:54Z

Ok, thanks for info. I just wanted to make sure that privacy is kept since most users only have 3 devices and adding a new device is a rare action.

calmh · 2017-06-01T08:21:48Z

I suggest doing something with what we have now, and potentially adding info to the discovery packets as a separate stage.

imsodin · 2017-06-01T08:38:11Z

I wonder how that looks on the fossdem wifi

That's why I chose a dropdown menu. What about combining @calmh's version with @AudriusButkevicius proposal, to make it somewhat "scalable":

I think it might be better to make some sort of expandable thing "Devices nearby" like advanced settings in folder editor that lists the devices.

AudriusButkevicius · 2017-06-01T08:49:28Z

Another way to do this would be to have a separate item in the settings dropdown menu (with a bootstrap badge listing the count of devices found and also only visible if the count is non zero) which shows a modal dialog with devices nearby. As you click on one of them, it shows you the add device dialog.

Another question, does this hide devices that are already added?

uok · 2017-06-01T09:14:55Z

You could also add a blue link (like "help" on other settings) next to "Device ID" which shows a (default hidden) div with the list once you click it. But I suggest to keep it simple. It would be great success for Syncthing to see a long list of Syncthing devices around them but at the moment I expect max. 5 devices to show up in this list. And only if the user has configured network, etc. correctly

calmh · 2017-06-09T07:27:55Z

Any collective decision here or 🖌🚲🏡 ?

AudriusButkevicius · 2017-06-09T07:45:43Z

What you suggested is better than whats in the PR if we don't want to go for something more advanced.

uok · 2017-06-09T08:14:42Z

I think @calmh's idea is simple and fast: click/tap and done.

imsodin · 2017-06-09T09:49:51Z

Done. I am still concerned that users from the top 5% in the usage statistics may have to do quite some scrolling on adding a device, but maybe that's not really an issue.

calmh · 2017-06-09T09:58:12Z

We should not show devices here that are already added, which mitigates that to a certain extent.

calmh · 2017-06-09T09:59:05Z

Also I think my idea to show the addresses needs to be somewhat restrained, at the least to not show relay addresses...

AudriusButkevicius · 2017-06-09T09:59:42Z

I don't think we should show addresses, and cap at max 5 unadded devices.

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

Works for me. Extra points for a ... and {{items.length - 5}} other devices or something under the list if it overflows. But really, we're almost never going to need it.

imsodin · 2017-06-09T10:56:31Z

I forgot it in the commit message: Already connected devices are filtered out as well.

AudriusButkevicius · 2017-06-13T20:03:42Z

so looks good to me, someone who actually runs more than one syncthing should test.

AudriusButkevicius · 2017-06-13T20:03:49Z

@st-review lgtm

st-review · 2017-06-13T20:03:50Z

@AudriusButkevicius: Noted! Need another LGTM or explicit merge command.

calmh · 2017-06-16T15:27:03Z

@st-review lgtm

st-review · 2017-06-16T15:27:12Z

👌 Merged as 98cfc20. Thanks, @imsodin!

…ixes #4157) GitHub-Pull-Request: #4186 LGTM: AudriusButkevicius, calmh

dinosore · 2017-06-29T11:38:21Z

For me, the list shows an already connected device. v0.14.32-rc.1, Linux (64 bit).

GitHub-Pull-Request: #4239

…ixes syncthing#4157) GitHub-Pull-Request: syncthing#4186 LGTM: AudriusButkevicius, calmh

GitHub-Pull-Request: syncthing#4239

gui: Add dropdown menu to choose discovered devices to device modal (f…

fb12b2e

…ixes syncthing#4157)

AudriusButkevicius reviewed May 31, 2017

View reviewed changes

Remove .isEmptyObject and polish

d4ea648

List of devices

c66d584

Don't show addresses and limit to 5 entries

ec0b645

st-review closed this Jun 16, 2017

st-review pushed a commit that referenced this pull request Jun 16, 2017

gui: Add dropdown menu to choose discovered devices to device modal (f…

98cfc20

…ixes #4157) GitHub-Pull-Request: #4186 LGTM: AudriusButkevicius, calmh

imsodin added a commit to imsodin/syncthing that referenced this pull request Jun 29, 2017

gui: Fix discovered devices list (follow-up to syncthing#4186)

88d5813

st-review pushed a commit that referenced this pull request Jun 29, 2017

gui: Fix discovered devices list (follow-up to #4186)

8d13e01

GitHub-Pull-Request: #4239

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

gui: Add dropdown menu to choose discovered devices to device modal (f…

0cd56cb

…ixes syncthing#4157) GitHub-Pull-Request: syncthing#4186 LGTM: AudriusButkevicius, calmh

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

gui: Fix discovered devices list (follow-up to syncthing#4186)

6dcf314

GitHub-Pull-Request: syncthing#4239

imsodin deleted the fix-4157 branch October 5, 2017 18:24

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 15, 2018

syncthing locked and limited conversation to collaborators Jul 15, 2018

gui: Add dropdown menu to choose discovered devices to device modal (fixes #4157) #4186

gui: Add dropdown menu to choose discovered devices to device modal (fixes #4157) #4186

Conversation

imsodin commented May 31, 2017

Purpose

Testing

Screenshots

Implementation

AudriusButkevicius May 31, 2017

Choose a reason for hiding this comment

AudriusButkevicius May 31, 2017

Choose a reason for hiding this comment

imsodin May 31, 2017

Choose a reason for hiding this comment

AudriusButkevicius May 31, 2017

Choose a reason for hiding this comment

imsodin May 31, 2017

Choose a reason for hiding this comment

AudriusButkevicius May 31, 2017

Choose a reason for hiding this comment

imsodin May 31, 2017

Choose a reason for hiding this comment

AudriusButkevicius commented May 31, 2017 • edited

imsodin commented May 31, 2017

AudriusButkevicius commented May 31, 2017

calmh commented Jun 1, 2017

AudriusButkevicius commented Jun 1, 2017

calmh commented Jun 1, 2017

AudriusButkevicius commented Jun 1, 2017 • edited

uok commented Jun 1, 2017

AudriusButkevicius commented Jun 1, 2017 • edited

uok commented Jun 1, 2017

calmh commented Jun 1, 2017

imsodin commented Jun 1, 2017

AudriusButkevicius commented Jun 1, 2017

uok commented Jun 1, 2017 • edited

calmh commented Jun 9, 2017

AudriusButkevicius commented Jun 9, 2017

uok commented Jun 9, 2017

imsodin commented Jun 9, 2017

calmh commented Jun 9, 2017

calmh commented Jun 9, 2017

AudriusButkevicius commented Jun 9, 2017 • edited

calmh commented Jun 9, 2017

imsodin commented Jun 9, 2017

AudriusButkevicius commented Jun 13, 2017

AudriusButkevicius commented Jun 13, 2017

st-review commented Jun 13, 2017

calmh commented Jun 16, 2017

st-review commented Jun 16, 2017

dinosore commented Jun 29, 2017

AudriusButkevicius commented May 31, 2017 •

edited

AudriusButkevicius commented Jun 1, 2017 •

edited

AudriusButkevicius commented Jun 1, 2017 •

edited

uok commented Jun 1, 2017 •

edited

AudriusButkevicius commented Jun 9, 2017 •

edited