Feature: Add sysproxy to fyne-proxy example
#214
Conversation
|
@fortuna this is a small change to add |
x/examples/fyne-proxy/main.go
Outdated
| @@ -126,6 +127,7 @@ func makeAppHeader(title string) *fyne.Container { | |||
| } | |||
|
|
|||
| func main() { | |||
| defer sysproxy.DisableWebProxy() | |||
There was a problem hiding this comment.
Disable on runningProxy.Close() instead.
Perhaps have a defer that checks if proxy != nil and closes it.
Move it to where the proxy variable is defined.
x/examples/fyne-proxy/main.go
Outdated
| log.Printf("Failed to parse address: %v\n", err) | ||
| } | ||
| // Set system-wide proxy settings | ||
| err = sysproxy.SetWebProxy(host, port) |
There was a problem hiding this comment.
Move the logic into runServer. And you need to check if you are on Linux, Windows or macOS in order to set the system proxy. We should output something in the status saying we set the system proxy.
There was a problem hiding this comment.
@fortuna I will move this to runserver. the built system will automatically uses GOOS in the go build flags and picks the right platform.
There was a problem hiding this comment.
I know, but we should not be calling those methods on the unsupported platforms.
And we should probably report an error if the system proxy fails to be set.
There was a problem hiding this comment.
when the platform is not supported (ios and android for example) sysproxy_other.go file in the sysproxy package runs, and it returns an error: https://github.com/Jigsaw-Code/outline-sdk/blob/main/x/sysproxy/sysproxy_other.go
Thinking a bit more about moving SetWebProxy() logic into runServer, I believe it's better if we keep this logic separate since it essentially starts a local server listening on a port.
SetWebProxy is a separate logic. In the current code, I start the server and then set system settings to point to it. If you run this on Android for example, the local server still keeps listening (as long as on foreground) but SetWenProxy() returns a not supported platform error. This is just a PoC and the blazer proxy fork will improve on error handling UI.
Others can also use the port exposed by the proxy server locally. I can see cases where setting system proxy is not needed. Keeping these logics separate provides extra decoupling.
There was a problem hiding this comment.
For this application, the runServer function is the logic of the start button. I'd like to keep it that way, so UI and business logic a more clearly defined.
Running the proxy commands when it's not supported is a bad practice. In real apps they need to know the support, in order to adapt the UI and application logic.
We need a function that tells us whether the System Proxy can be configured.
If it can be configured, we should indicate success or failure in the status line.
If it cannot, we show nothing else other than the current status.
There was a problem hiding this comment.
@fortuna I created a new logic called startProxy and endProxy to ensure UI and proxy logic are decoupled. startProxy encapsulate runServer and SetWebProxy logic.
I like your suggestion on having aIsSupported method to check if System Proxy can be configured before even attempting it. This should be added to sysproxy package and I can do that via a separate PR.
As a short-term solution I use error.Is to check if the SetWebProxy is returning platform is not supported error.
There was a problem hiding this comment.
@fortuna Not sure if you saw the latest changes here. I appreciate your feedback on this whenever you have a moment to review the changes.
x/examples/fyne-proxy/main.go
Outdated
| } else { | ||
| // Disable system-wide proxy |
There was a problem hiding this comment.
Do this in Close() instead.
x/examples/fyne-proxy/main.go
Outdated
| log.Printf("Failed to parse address: %v\n", err) | ||
| } | ||
| // Set system-wide proxy settings | ||
| err = sysproxy.SetWebProxy(host, port) |
There was a problem hiding this comment.
For this application, the runServer function is the logic of the start button. I'd like to keep it that way, so UI and business logic a more clearly defined.
Running the proxy commands when it's not supported is a bad practice. In real apps they need to know the support, in order to adapt the UI and application logic.
We need a function that tells us whether the System Proxy can be configured.
If it can be configured, we should indicate success or failure in the status line.
If it cannot, we show nothing else other than the current status.
This PR adds a new feature to
fyne-proxyexample that sets up the system-wide web proxy that tunnels web browser traffic automatically on following platforms: