Show discovery errors in modal instead of tooltip (fixes #2344)

[sacheendra]

Purpose

Previously, the errors relating to discovery were displayed in a tooltip as mentioned in #2344. Apart from being ugly, the errors were could not be copied.

This replaces the tooltip with a modal. Text is added to the tooltip to inform the user that clicking the text will show the errors. The errors are displayed as an unordered list inside the modal.

Questions: Are the title text and tooltip text ok? Is the red colour with the danger icon appropriate for the modal?

Testing

Tested with different viewport sizes by changing the dimensions of the chrome viewport. Checked if the text in the modal can be copied.

Screenshots

*The new tooltip*

*The discovery errors modal*

[AudriusButkevicius]

Great addition, thanks.

What's the rationale for changing the text colors? I think having it red/green/etc was fine?

Also, I wonder if we should change the format of the errors so that they describe better which address is failing etc, so we could have a table or some more structured representation in the UI.

[canton7]

Nit: Can we call them "failures" or something, rather than "errors"? Not being able to do IPv6 discovery because you don't have an IPv6 connection is not an error, and it's not something the user can fix.

[sacheendra]

@canton7 Calling them failures would be better. But, the only way to know wether they are failures or errors would be to do a string match on the errors which doesn't seem very robust. If there is a better way, I can implement that.

@AudriusButkevicius The red and green should still work as intended. Green when all connections are successful and red when none are. The colour change is for the case when some succeed. As I made it into a link, the text would have the light blue colour used for links. I set the colour to black to retain the same look as before.

This just looked odd.

[sacheendra]

Maybe it is unrelated, but turning my internet off to test wether it goes to red was not possible. When I turned my internet off, it still showed that 1 discovery server was reachable. How is that possible?

[AudriusButkevicius]

Local discovery happens locally.
Global discovery happens once every X minutes.

[canton7]

Depends on what you mean by turning your Internet off. If you were still connected to a network, Syncthing will have done local discovery. If you disconnected from all networks / disabled the network device, no idea...

[sacheendra]

I'm not connected to any network. But local discovery was still working I guess. I can produce the case where there are no discovery servers by disabling local discovery in the settings and also disconnecting from the network.

[calmh]

Yeah, or without the pain of losing internet: ```diff </gui> <options> <listenAddress>kcp://127.0.0.1:22001</listenAddress> - <globalAnnounceServer>default</globalAnnounceServer> + <globalAnnounceServer>https://example.com/</globalAnnounceServer> <globalAnnounceEnabled>false</globalAnnounceEnabled> <localAnnounceEnabled>true</localAnnounceEnabled> <localAnnouncePort>21027</localAnnouncePort> ``` in your config xml will fail global discovery. You'll see it as just one service then, as the normal "default" expands to a set of six servers.

[sacheendra]

The colour for the text is now inherited from the parent instead of being statically set.

[AudriusButkevicius]

So the errors -> failures would still be nice.

[AudriusButkevicius]

What does the colors look like now?

[sacheendra]

Red coloured text when all discovery fails.

Green coloured text when all discovery succeeds.

Black when there are a few failures.

[sacheendra]

@calmh Weirdly, https://example.com actually causes discovery to succeed. But https://foobar.com causes it to fail as expected.

[sacheendra]

Testing

Notice the text "failures".

[canton7]

(Just to make sure: the suggestion isn't to display "errors" or "failures" depending in the content if the dialog. It is to just use the word "failures" (or something else if someone has a better suggestion) instead of "errors")

[canton7]

... And great job putting this together. I've been annoyed by that tooltip as well, but too lazy to do anything about it...

[AudriusButkevicius]

Also, we could add a note at the modal saying that IPv6 failures if you do not IPv6 connectivity is expected etc.

[sacheendra]

Testing

Notice reagrding IPv6 failures.

This doesn't have translations though. Not sure how to add them.

[calmh]

For translations, just add a `translate` attribute to the containing tag. <span translate>Some English text here</span> Then run "go run build.go translate" to add it to the lang-en.json. I've added you to AUTHORS for when this is complete.

[calmh]

I think possibly the notice is fine, but too prominent. It doesn't need to be top and center, I'd de-emphasize it as a note below any errors.

[sacheendra]

Testing

This is how the info. looks like now.

[calmh]

@st-review lgtm (commit message will need to be tweaked)

[st-review]

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

[AudriusButkevicius]

@st-review lgtm

gui: Move discovery failures to modal (fixes #2344)

[st-review]

👌 Merged as ee92ee0. Thanks, @sacheendra!

[AudriusButkevicius]

Ghrr wrong button