search

aequitas / macos-menubar-wireguard

macOS menubar icon for WireGuard/wg-quick
GNU General Public License v3.0
227 stars 38 forks source link

Display ping results for recently-used servers #6

Closed eecharlie closed 5 years ago

eecharlie commented 5 years ago

(Feature request)

Some clients, such as VyprVpn's, show recent ping results in the menu of server locations, as a heuristic for choosing a server likely to perform well. This could be done in the main menu here.

This is helpful when actually using a VPN in a repressive internet context like China, or just to access the internet as if in a far-away country.

This may be too resource-intensive to automatically generate for every server configuration, but could be paired with a duplicated top listing of last 5 servers used, and only displayed for the last 5 servers.

aequitas commented 5 years ago

Good idea. This could probably be enabled on a per vpn basis in a settings panel (if I had one). So it would only drain sources if you want to.

However I'm gonna put this on a low priority for now as there are some other issue I'd like to tackle first.

If you like to have a go at implementing it yourself I'd welcome a PR. However keep in mind the current codebase might be refactored the upcoming weeks. If you can keep the code in a separate class you should be fine.

eecharlie commented 5 years ago

Unlikely I have time to learn Swift, but supposing I did: would it be sensible to add a field to e.g. the Peer or Tunnel struct for ping_avg, or should it be a totally separate structure that is a dict of IP addresses mapping to ping results?

What event should trigger a refresh of the ping values? Startup/file change detection, and menu pull-down?

aequitas commented 5 years ago

I'm learning Swift as I'm getting along with this project, so don't worry ;).

I would prefer the separate structure mapping Tunnel to ping configuration (enabled/disabled/threshold) and results. As ping times are likely to change during runtime I think a timer interval would be best as trigger. Maybe something like https://developer.apple.com/documentation/foundation/timer ? Menu pull would impact UX to much as it would introduce a delay.

eecharlie commented 5 years ago

Thinking about use cases, the ping is not informative while a vpn connection is active because it will be pinging vpn servers from one of the other vpn servers.

The user wants to know ping times from their actual location to a candidate server when they are selecting the most likely to have a low-latency connection. So I would say a single-pass refresh of ping times should be triggered upon a disconnect is the simplest case. Right?

Because it may be relevant, what's your status on detecting and maintaining state information of whether a connection is successful, as opposed to just the active configuration?

Finally, what's your source code license? I don't think I've contributed to a project before that just said "all rights reserved" in every source file and didn't have a separate license file clarifying open source.

aequitas commented 5 years ago

Pinging on disconnect would still result in stale information. I think in that case it might be best to make it user triggerable with a button or the like.

Detecting connection status is a feature I intend to implement soon. Haven't researched yet what would be the best way or what wg exposes in terms of API to provide this info. Maybe the last handshake time is unable, or a ping heartbeat over the line might be needed?

Licence is in progress. I have some code borrowed from sources where I don't know the licence of. So I want to get that sorted first before I put a licence on the repo. Will probably end up with GPL. The current header on sources is a Xcode default template, haven't looked at that yet :)

eecharlie commented 5 years ago

Hmm, why would it result in stale information? It would be stale soon after you've connected, but I don't think you'd need the information anymore.

Or is your concern the information becoming stale soon after disconnection? I agree, I'm just suggesting that that's the simplest starting point, and then a timer or UX trigger can be added later.

I think you're right that handshake recency is a good way to monitor connection quality.

Should the UX prevent the user from adding a tunnel when one is already active? I'm unclear on whether this adds a second hop to the routing, or does something useless/unproductive.

aequitas commented 5 years ago

If you ping once after a few minutes/hours that information is no longer valid. A server might have gone away, be on a different (slower/faster) route etc. If we want to implement this we need to account for all use cases someone uses it in or it will just confuse users and be considered buggy. Feel free to experiment with implementing this functionality. I'm not yet inclined to include it just yet as I want to focus on getting a stable reliable MVP out as a first official release. But after that I'm glad to have this functionality included.

Multiple tunnels can run just fine. Depending on the allowedips settings not all traffic has to be routed through the tunnel. Also some people might want embedded tunnels.

aequitas commented 5 years ago

I'm not going to implement this as I have stopped development of features due to the official App being available.