apache / cordova-plugin-network-information

Apache Cordova Network Information Plugin
https://cordova.apache.org/
Apache License 2.0
464 stars 321 forks source link

feat(android, ios): add 5G support #130

Open ZumelzuR opened 3 years ago

ZumelzuR commented 3 years ago

Platforms affected

Motivation and Context

  Solve 5g detections for iOS and Android (the current state of the plugin classify as UKNOWN the 5g network)
 open issue -> https://github.com/apache/cordova-plugin-network-information/issues/125

closes #125

Description

 We added a a new listener for check if NR is available and using that, if is the case, detect the 5g.

Testing

    We tested on android simulator and a physical android 10 and iOS simulator

Checklist

dev-jcb commented 3 years ago

How do we get pull requests to get merged in master branch?

almothafar commented 3 years ago

Any news about when to release this fix

ZumelzuR commented 3 years ago

Thank, these days are a little problematic for me but I will check and resolve all as soon as possible

ZumelzuR commented 2 years ago

Sorry was impossible to me to do this changes before. I update all as you requested and tested in my local. If you have more feedback or changes to me add let me know

ZumelzuR commented 2 years ago

You are right. Sorry I didn't realized about the changes on the package-lock.json. I will do the revert now

ZumelzuR commented 2 years ago

what about this?

DavidWiesner commented 2 years ago

@ZumelzuR On my iPhone 13 Pro networkInfo.currentRadioAccessTechnology is always nil. Strange enough another phone (with the same os version) this pointer is not nil. I found networkInfo.currentRadioAccessTechnology is deprecated and can be nil on ios >= 14.2. I`ve found this fix https://github.com/Tencent/Hippy/pull/1597/files. May you can try this and add this to your PR.

Edit: May you will use a more reactive form (with a nil check):

static NSString *radioAccessNameIn(CTTelephonyNetworkInfo *networkInfo) {
  if (@available(iOS 13.0, *)) {
    if (networkInfo.currentRadioAccessTechnology == nil && networkInfo.dataServiceIdentifier) {
        return [networkInfo.serviceCurrentRadioAccessTechnology objectForKey:networkInfo.dataServiceIdentifier]; 
    }
  }
  return networkInfo.currentRadioAccessTechnology;
}
ZumelzuR commented 2 years ago

@ZumelzuR On my iPhone 13 Pro networkInfo.currentRadioAccessTechnology is always nil. Strange enough another phone (with the same os version) this pointer is not nil. I found networkInfo.currentRadioAccessTechnology is deprecated and can be nil on ios >= 14.2. I`ve found this fix https://github.com/Tencent/Hippy/pull/1597/files. May you can try this and add this to your PR.

Edit: May you will use a more reactive form (with a nil check):

static NSString *radioAccessNameIn(CTTelephonyNetworkInfo *networkInfo) {
  if (@available(iOS 13.0, *)) {
    if (networkInfo.currentRadioAccessTechnology == nil && networkInfo.dataServiceIdentifier) {
        return [networkInfo.serviceCurrentRadioAccessTechnology objectForKey:networkInfo.dataServiceIdentifier]; 
    }
  }
  return networkInfo.currentRadioAccessTechnology;
}

Thank you, ok yes I will do some testing on my devices and come back with the fix and an update.

ZumelzuR commented 2 years ago

Done, I added the fixes at https://github.com/apache/cordova-plugin-network-information/pull/130/commits/b65a78315c6ed5e309fc639a2c02f9069d49505e

DavidWiesner commented 2 years ago

I got an error

network-information/CDVConnection.m:63:94: expected ';' at end of declaration

ZumelzuR commented 2 years ago

I got an error

network-information/CDVConnection.m:63:94: expected ';' at end of declaration

Sorry I forgot this, I added in the plugins folder but not here, I already add the fix commit.

mdivya-symplr commented 2 years ago

@ZumelzuR I see IOS Test suites are failing for this PR

ZumelzuR commented 2 years ago

For fix this I should try to simulate a 5g connection in a ios12 device and check if is working?

ZumelzuR commented 2 years ago

@breautek I review the commits and add the changes of @erisu

thanks

ZumelzuR commented 2 years ago

some update on this?

ZumelzuR commented 2 years ago

Can somebody run the test or merge finally this? @erisu @breautek

breautek commented 1 year ago

closed/re-opened the PR as the test results expired and wasn't available. Assuming that the tests passes I'll do one last row call and I'll merge if there are no objections in the next ~24 hours.