-
Notifications
You must be signed in to change notification settings - Fork 124
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
Fixed issue #243 and improved Auto Join #256
base: master
Are you sure you want to change the base?
Conversation
Ready for review |
All these changes should make the Auto Join work correctly. |
After many tests, it is now truly ready for review. |
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.
Hi, thanks again for the contribution. Please address the comments.
DispatchQueue.global().async { | ||
CredentialsManager.instance.setAutoJoin(ssid, false) |
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.
Please correct me if I'm wrong, I did recall this is what Apple's implementation used to do back in the days - manually disconnecting a network will opt-out from auto-join, although this is not what the latest macOS versions are doing.
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.
You are correct. Apple’s implementation used to work this way: manually disconnecting from a network would remove it from auto-join. However, more recent versions of macOS no longer follow this behavior; Apple made this change to improve user experience by reducing the need to manually reconfigure network preferences.
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.
Would you prefer to keep this behavior of automatically disabling autojoin when the user disconnects? I think this behavior is not very interesting and many users may not like this behavior. If you prefer to keep this behavior, I think it would be better to add an option in the preferences to activate and deactivate this behavior.
@@ -59,7 +59,6 @@ final class NetworkManager { | |||
if let savedNetworkAuth = CredentialsManager.instance.get(networkInfo) { | |||
networkInfo.auth = savedNetworkAuth | |||
Log.debug("Connecting to network \(networkInfo.ssid) with saved password") | |||
CredentialsManager.instance.setAutoJoin(networkInfo.ssid, true) |
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’m not certain if this is what Apple’s implementation used to do back in the days (feels unlikely and doesn't match with the current implementation). This is the root cause of #243.
HeliPort/Appearance/StatusMenu.swift
Outdated
@@ -43,6 +46,11 @@ final class StatusMenu: NSMenu, NSMenuDelegate { | |||
|
|||
statusItem.title = NSLocalizedString(status.description) | |||
|
|||
if status != ITL80211_S_RUN && previousStatus == ITL80211_S_RUN && !isDisconnecting { | |||
disassociateSSID(disconnectItem) | |||
NetworkManager.scanSavedNetworks() |
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.
Currently we're only performing auto-join when HeliPort is initially launched. Users might want to temporarily disconnect while keeping WiFi enabled. Apple’s approach supports this (likely to maintain BSSID-based location services). Although there’s minimal justification for us to replicate this behavior aside from consistency with Apple (aligns with user habits and expectations) and allowing users to view the network list while disconnected. See #133.
HeliPort/Appearance/StatusMenu.swift
Outdated
@@ -43,6 +46,11 @@ final class StatusMenu: NSMenu, NSMenuDelegate { | |||
|
|||
statusItem.title = NSLocalizedString(status.description) | |||
|
|||
if status != ITL80211_S_RUN && previousStatus == ITL80211_S_RUN && !isDisconnecting { |
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.
The conditions here are not robust enough. For example, in OpenIntelWireless/itlwm#934 (comment) the router requests a reassociation. If we caught this event, we might mistakenly disconnect from the current network.
Many other factors might cause a change in state as well, such as firmware crash (goes directly back to S_INIT
, no intervention from HeliPort required, itlwm will recover by itself), key expiring (-> S_AUTH
), etc. The idea is to have our own state adapt and tolerate well with itlwm's internal state.
HeliPort/Appearance/StatusMenu.swift
Outdated
@@ -30,9 +30,12 @@ final class StatusMenu: NSMenu, NSMenuDelegate { | |||
private var networkListUpdateTimer: Timer? | |||
private var statusUpdateTimer: Timer? | |||
|
|||
private var isDisconnecting: Bool = false |
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'm guessing this flag is to prevent race conditions? Justification will be appreciated.
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.
This flag serves so that when the user manually disconnects from the network, the condition on line +49 will not attempt to disconnect again and will not call NetworkManager.scanSavedNetworks(), thus allowing the user to remain disconnected.
HeliPort/Appearance/StatusMenu.swift
Outdated
DispatchQueue.main.asyncAfter(deadline: .now() + 2.0) { | ||
self.isDisconnecting = false | ||
Log.debug("Disconnected from \(ssid)") | ||
} |
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.
Justification required here as well.
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.
These lines serve so that when the user disconnects, the isDisconnecting
flag will be true for 2 seconds, thereby preventing the condition on line +49 from attempting to disconnect again and calling NetworkManager.scanSavedNetworks(). This allows the user to remain disconnected, as when isDisconnecting
is true, the condition always returns false and does not execute what is inside it.
if status == ITL80211_S_RUN { | ||
disassociateSSID(disconnectItem) | ||
} |
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 connected, it will disconnect from the current network before turning off the Wi-Fi, thus preventing it from connecting to a network with auto-join disabled when Wi-Fi is turned on (if the previously connected network has auto-join disabled).
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.
With the changes made to .turnWiFiOn
and .turnWiFiOff
, it behaves the same as Apple's implementation, at least in more recent versions of macOS, as I'm not sure if the same happens in earlier versions.
@@ -435,7 +435,11 @@ final class StatusMenu: NSMenu, NSMenuDelegate { | |||
} | |||
case .turnWiFiOn: | |||
power_on() | |||
NetworkManager.scanSavedNetworks() |
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 Wi-Fi is turned on, it will attempt to auto-join.
Fixes for the issue #243