Skip to content
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

Issue #2052 - Change reachability computation #2060

Closed
wants to merge 2 commits into from
Closed

Issue #2052 - Change reachability computation #2060

wants to merge 2 commits into from

Conversation

MarcoSantarossa
Copy link
Contributor

@MarcoSantarossa MarcoSantarossa commented Apr 3, 2017

It fixes the issue #2052. There is a bug with the reachability computation when the user has a sim but s/he didn't select the carrier yet. In this scenario we have both .connectionRequired and .isWWAN.


private func isNetworkReachable(with flags: SCNetworkReachabilityFlags) -> Bool {
let needsConnection = flags.contains(.connectionRequired)
let canConnectionAutomatically = flags.contains(.connectionOnDemand) || flags.contains(.connectionOnTraffic)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this name be canConnectAutomatically ?

if flags.contains(.connectionOnDemand) || flags.contains(.connectionOnTraffic) {
if !flags.contains(.interventionRequired) { networkStatus = .reachable(.ethernetOrWiFi) }
}
let isNetworkReachable = self.isNetworkReachable(with: flags)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you combine this with the guard statement above?

into:

 guard flags.contains(.reachable), isNetworkReachable(with:flags) else { return .notReachable }

@MarcoSantarossa
Copy link
Contributor Author

MarcoSantarossa commented Apr 14, 2017

I think CI for watchOS has some issues @jshier

@MarcoSantarossa
Copy link
Contributor Author

@jshier can you rerun the tests please? They pass locally so it seems an odd failure.

@jshier
Copy link
Contributor

jshier commented May 2, 2017

No problem, it's a Travis issue (still).

@MarcoSantarossa
Copy link
Contributor Author

@jshier is this PR under review? Or Don't you want it merged?

@jshier
Copy link
Contributor

jshier commented May 29, 2017

@MarcoSantarossa We're still reviewing changes for our 4.5 release, we'll likely review and merge soon.

cnoon pushed a commit that referenced this pull request Jun 16, 2017
…WAN] combo.

These changes resolve issue #2052 where the reachability calcuation was incorrectly reporting the connection as reachable. When a user has install a simcard, but hasn’t yet selected a carrier, the connection is not reachable.
@cnoon
Copy link
Member

cnoon commented Jun 16, 2017

Thanks for putting this together @MarcoSantarossa...much appreciated! 🍻 I've made a few small tweaks to your PR and have pushed it into master in 68f4eb3 while maintaining your attribution. This change will go out shortly in the AF 4.5 release.

Thanks again!

@cnoon cnoon closed this Jun 16, 2017
@cnoon cnoon added this to the 4.5.0 milestone Jun 16, 2017
@cnoon cnoon self-assigned this Jun 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants