-
Notifications
You must be signed in to change notification settings - Fork 41
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
Feature: Add sysproxy
to fyne-proxy
example
#214
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-proxy
example that sets up the system-wide web proxy that tunnels web browser traffic automatically on following platforms: