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: Remove probing for remote GUI address (ref #7017) #7136

Merged
merged 2 commits into from Nov 24, 2020

Conversation

calmh
Copy link
Member

@calmh calmh commented Nov 20, 2020

This removes the probing for the remote side, instead making it so we
simply construct the address based on the currently connected address,
if any, and let the user sort out whether it works or not.

This removes the probing for the remote side, instead making it so we
simply construct the address based on the currently connected address,
if any, and let the user sort out whether it works or not.
imsodin
imsodin previously approved these changes Nov 20, 2020
Copy link
Member

@acolomb acolomb left a comment

Choose a reason for hiding this comment

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

Looks good overall, just some wording suggestions.

<a ng-if="idToRemoteGUI[deviceCfg.deviceID]" href="{{idToRemoteGUI[deviceCfg.deviceID].replace('%', '%25')}}">{{idToRemoteGUI[deviceCfg.deviceID]}}</a>
<span ng-if="!idToRemoteGUI[deviceCfg.deviceID]">Unreachable</span>
<a ng-if="hasDeviceGUIAddr(deviceCfg)" href="{{deviceGUIAddr(deviceCfg).replace('%', '%25')}}">{{deviceGUIAddr(deviceCfg)}}</a>
<span ng-if="!hasDeviceGUIAddr(deviceCfg)">Unreachable</span>
Copy link
Member

Choose a reason for hiding this comment

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

If we don't do any probing, I don't think that "Unreachable" is a sensible information. If you mean to indicate there is no suitable connection to construct an address from, maybe we should change the wording. Something like "No connection"?

And I thought the discussion turned out in favour of the "remote GUI" naming, also for variables / functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps "Unknown"?

Copy link
Member

@acolomb acolomb Nov 21, 2020

Choose a reason for hiding this comment

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

Actually we do know. Either there is no connection, or it's known not to be related to the host itself, but a relay server. So "No connection" is still my favorite, although for the second case "No direct connection" would be more accurate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, there is a remote device and it has a GUI, we just don't know how to connect to it. We also use it for other things like unknown folder status so it's already there and translated. "No [direct] connection" would be more like the underlying reason we don't know the GUI address.

Copy link
Member

Choose a reason for hiding this comment

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

But the heading "Remote GUI" is not about an address. The fact is that we do not know how to (hopefully) reach it, which is somewhere between "Unreachable" and "Unknown". Maybe get more explicit and say "Address unknown"?

gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
@imsodin imsodin merged commit 6864f7c into syncthing:main Nov 24, 2020
@acolomb
Copy link
Member

acolomb commented Nov 24, 2020

My last review comment was not addressed yet. Please reconsider the wording because an "Unknown" remote GUI is not a sensible information.

@calmh
Copy link
Member Author

calmh commented Nov 24, 2020

Well, I at least disagree, I think it's fine and quite unambiguous.

@calmh calmh deleted the noprobe branch November 24, 2020 21:11
@calmh calmh added this to the v1.12.1 milestone Nov 24, 2020
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Nov 25, 2021
@syncthing syncthing locked and limited conversation to collaborators Nov 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants