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

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

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Conversation

ZumelzuR
Copy link

@ZumelzuR ZumelzuR commented May 13, 2021

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

  • [ x ] I've run the tests to see all new and existing tests pass
  • [ x] I added automated test coverage as appropriate for this change
  • [x ] Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • [ x] If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • [x ] I've updated the documentation if necessary

@ZumelzuR ZumelzuR mentioned this pull request May 13, 2021
@dev-jcb
Copy link

dev-jcb commented Jul 7, 2021

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

@almothafar
Copy link

Any news about when to release this fix

Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

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

Thank you for your PR.

I've added some notes, if you have any questions feel free to ask.

Noting that due to the added dependency of androidx this will be considered a breaking change for the plugin. This isn't a bad thing, just simply a note for maintainers, where this plugin will require at least cordova-android@9 (with AndroidXEnabled preference enabled).

package.json Outdated Show resolved Hide resolved
plugin.xml Outdated Show resolved Hide resolved
src/android/NetworkManager.java Outdated Show resolved Hide resolved
src/android/NetworkManager.java Outdated Show resolved Hide resolved
src/android/NetworkManager.java Show resolved Hide resolved
src/android/NetworkManager.java Outdated Show resolved Hide resolved
src/android/NetworkManager.java Show resolved Hide resolved
@ZumelzuR
Copy link
Author

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

@ghost ghost mentioned this pull request Feb 22, 2022
@ZumelzuR
Copy link
Author

ZumelzuR commented Apr 7, 2022

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

Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

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

Thanks for making the requested changes.

One last thing, the package-lock change should also be reverted, if possible. package-lock updates are typically done in their own independent PRs. This way if we ever had to revert this PR in the future, the revert won't be touching the package-lock.

As for the code changes, I've had a glance over and everything looks ok to me. I've approved the test actions to run and hopefully I can find some time tomorrow night to give it a test in an emulator.

@ZumelzuR
Copy link
Author

ZumelzuR commented Apr 8, 2022

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

@ZumelzuR ZumelzuR requested a review from breautek April 11, 2022 14:22
@ZumelzuR
Copy link
Author

what about this?

APIs are only available in 14.1, not in 14.0

Co-authored-by: David Boho <[email protected]>
Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

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

lgtm

@breautek breautek requested review from PieterVanPoyer and erisu June 14, 2022 16:31
@DavidWiesner
Copy link

DavidWiesner commented Jun 15, 2022

@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
Copy link
Author

@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
Copy link
Author

Done, I added the fixes at b65a783

@DavidWiesner
Copy link

I got an error

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

@ZumelzuR ZumelzuR closed this Jun 22, 2022
@ZumelzuR ZumelzuR reopened this Jun 22, 2022
@mdivya-symplr
Copy link

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

@ZumelzuR
Copy link
Author

ZumelzuR commented Oct 5, 2022

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

@breautek breautek closed this Oct 13, 2022
@breautek breautek reopened this Oct 13, 2022
src/ios/CDVConnection.m Outdated Show resolved Hide resolved
@erisu erisu changed the title 5g feature feat(android, ios): add 5G support Oct 13, 2022
Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

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

Just nitpick changes to cleanup the PR.

  • Cordova uses a 4-space indention.

src/android/NetworkManager.java Outdated Show resolved Hide resolved
src/android/NetworkManager.java Outdated Show resolved Hide resolved
src/android/NetworkManager.java Outdated Show resolved Hide resolved
src/android/NetworkManager.java Outdated Show resolved Hide resolved
src/android/NetworkManager.java Outdated Show resolved Hide resolved
src/android/NetworkManager.java Outdated Show resolved Hide resolved
src/android/NetworkManager.java Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
src/android/NetworkManager.java Outdated Show resolved Hide resolved
src/android/NetworkManager.java Outdated Show resolved Hide resolved
@ZumelzuR ZumelzuR requested review from erisu and removed request for PieterVanPoyer October 25, 2022 17:44
@ZumelzuR
Copy link
Author

ZumelzuR commented Oct 25, 2022

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

thanks

@ZumelzuR
Copy link
Author

ZumelzuR commented Nov 3, 2022

some update on this?

@ZumelzuR
Copy link
Author

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

@breautek breautek closed this Dec 16, 2022
@breautek breautek reopened this Dec 16, 2022
@breautek
Copy link
Contributor

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.

}
#if __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_14_1
else if (@available(iOS 14.1, *)) {
if ([currentRadioAccessTechnology isEqualToString:CTRadioAccessTechnologyNRNSA]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the preprocessor macro doesn't appear to be working as intended here cause iOS 13 tests is failing on this line due to CTRadioAccessTechnologyNRNSA not being available.

/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/tmp-3401-5V69QdTKGZQj/platforms/ios/HelloCordova/Plugins/cordova-plugin-network-information/CDVConnection.m:89:76: error: use of undeclared identifier 'CTRadioAccessTechnologyNRNSA'
                        if ([currentRadioAccessTechnology  isEqualToString:CTRadioAccessTechnologyNRNSA]) {
                                                                           ^

Copy link
Author

@ZumelzuR ZumelzuR Dec 22, 2022

Choose a reason for hiding this comment

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

@breautek this error not make much sense for me, I mean is in a block of code that if the version is iOS14 will try to access.

Also I read this error is related with the xcode

dcloudio/native-docs#30
https://stackoverflow.com/questions/71585983/use-of-undeclared-identifier-ctradioaccesstechnologynr

Any suggestions? I have the last version of xcode and I will try to run it in iOS 13 simulator and check if I can reproduce

Comment on lines +94 to 96
}
#endif
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is also causing a syntax error...

var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/tmp-3401-5V69QdTKGZQj/platforms/ios/HelloCordova/Plugins/cordova-plugin-network-information/CDVConnection.m:113:1: error: extraneous closing brace ('}')
}

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.

5G detection
8 participants