Open ailinykh opened 1 year ago
That UI oddity is possibly something more intentional than a bug. It's confusing because of the way the UI appears.
NewNode has an internal state of whether it is "enabled" or not. This is the user preference. It used to be represented with a toggle switch, which would only move when the user tapped it.
Independently, the OS has a notion of whether the VPN is "in use", represented as "Connected"/"Disconnected" in the UI.
So, it makes sense to have NewNode "enabled" but "disconnected", meaning the user wants to use NewNode when possible, but right now it's not possible.
(There's a further state one might call "Connected", regarding whether NewNode can reach peers, but this is not represented in the button or the status message.)
I think we could fix it by adding a double-check for the VPN switch enabled in preferences. The scenario is following:
.willEnterForegroundNotification
performs connectstartVPNTunnel
again
I made a review of the iOS App and there are some thoughts I would like to share with the community.
The ViewController is responsible for a lot of things:
This approach violates the Single Responsibility Principle and can lead to a Massive View Controller problem.
Let's see more precisely a ViewController call order:
You can see that some functions are calling another multiple times:
ViewDidLoad
->updateLayout
ViewDidLoad
->update
->NETunnelProviderManager.loadAllFromPreferences
->updateLayout
or
stateChanged
invokesNETunnelProviderManager.loadAllFromPreferences
and in the callback block, theupdate
method also invokes theNETunnelProviderManager.loadAllFromPreferences
This can lead to some wired UI bugs like so:
https://user-images.githubusercontent.com/939390/210129586-79cf0fc0-ae14-4981-97b3-5a035a3ee317.mp4
Solution
I suggest applying a Separation of concerns design principle and splitting ViewController to 4 components:
I can implement all of this, but I need the approval of the maintainers to make sure we have the same view of things.