CB-12035 (android) Fix bug [cordova-plugin-network-information] connection info is not reliable on Android 6

[PieterVanPoyer]

https://issues.apache.org/jira/browse/CB-12035 && #64: in case of TYPE_NONE (android bug?) return TYPE_UNKNOWN if ConnectivityManager.EXTRA_NO_CONNECTIVITY from the intent return false.

Platforms affected

android (6+)

What does this PR do?

For android M and above (6+) I check the EXTRA_NO_CONNECTIVITY flag. (https://developer.android.com/reference/android/net/ConnectivityManager.html#EXTRA_NO_CONNECTIVITY)
If the EXTRA_NO_CONNECTION flag still returns false, the connectionType is set to UNKNOWN.
This workaround dit not work for Lollipop androids. So the correction is only done for 6+ Androids.

What testing has been done on this change?

Thoroughly testing on my Samsung device (Oreo) and a Lollipop LG.

Checklist

• Reported an issue in the JIRA database
• Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
• Added automated test coverage as appropriate for this change.

[PieterVanPoyer]

Hello,

Can someone point me in the right direction why the build is failing.
Seems like the ios builds are failing, but i haven't touched the ios code.
I did only change the Java file.

Kind regards,
Pieter

[janpio]

You can ignore the platform=browser-safari one, that is currently globally broken and we are investigating how and why in apache/cordova-plugin-media-capture#105

The other one was stalled, I restarted it. Let's see if this fixes it.

[janpio]

Ok, failed again. The error message indicates that this is connnected to apache/cordova-cli#339, which is currently actively being worked on. Expect a CLI 8.1.1 release that will fix the issue.

[PieterVanPoyer]

I triggered a new build. (I did change some logging for a new commit).
The travis build is now successfull!
Are there any comments, questions, remarks for this pull request?

[PieterVanPoyer]

@janpio Hey Jan, should I do anything more for this pull request (request a review, merge, ...)? Or do I leave it like it is.

[janpio]

No, for now there is not really anything you can do - someone will have to come along and review it. (I'm currently working on fixing test failures across all plugin PRs, so I am more than busy - and also don't know much about Android) You could of course spend some time reviewing and commenting on other PRs of this plugin or others - that might get you some karma 💃

[joewoodhouse]

Is anyone able to merge this?

[janpio]

Did you test the change? Did it work? No side effects on older versions of Android?

[terreng]

I tested this on my Android 5 device, didn't work. I only tested this PR version because that's what I'm using in my prod app, so I'm not sure if it's these changes specifically that prevent it from working.

[PieterVanPoyer]

@terpro what is not working? Can you give some reproduction steps for the Android 5 device? Is the connection reported falsy as 'none'?

[terreng]

I don't have time to do extensive testing, but the online and offline events weren't being called

[PieterVanPoyer]

Just to be sure. Is it working with the master version of the cordova-plugin-network-information?

In production code, we do not rely on the online and offline events, we always read the:
navigator.connection.type === Connection.NONE
to determine whether the device is online or offline.

In my opinion this behaviour is not changed with this pull request.

[terreng]

I just tested both the master and the pr on my Android 5.1 device. navigator.connection.type works as expected in both cases. However, the online and offline events are not called in this pr, but are in master.

[PieterVanPoyer]

I just stumbled on this one: https://stackoverflow.com/questions/29677852/connectivitymanager-extra-no-connectivity-is-always-false-on-android-lollipop
My fix with using the EXTRA_NO_CONNECTIVITY flag does not work on lollipop. So, I need to make some adjustments.

[PieterVanPoyer]

@terpro end @joewoodhouse feel free to test it again. I rewrote it to use a networkCallback as suggested in the android documentation.
I will test it again tomorrow on my lollipop LG.

[PieterVanPoyer]

Back to the drawing table. Tests on my Oreo and LG are not successfull.

[janpio]

(I added [WIP] to the PR title to indicate that work on this is still ongoing. Just remove it when you have something that could and should be reviewed. Thanks.)

[terreng]

@PieterVanPoyer The changes you made originally fixed the issue for Android 6+ devices, but broke anything below 6. Is it possible to just add an API level/Android version check and use the old or new code based on that?

[PieterVanPoyer]

@terpro I followed your suggestion and introduced the Android Sdk version in the algorithm. So in case the sdk is below M - android the old behaviour is done.

[terreng]

@janpio @PieterVanPoyer Tested and working as expected on Android 5, 7, 8, 9, looks ready to merge

[PieterVanPoyer]

Great testing!
Should someone bump the version?
From 2.0.2-dev to 2.1.0 or to 2.0.3 and publish it after merging?

[janpio]

version is managed out of PRs in the release process.

[PieterVanPoyer]

Hey @janpio can it be merged? Who can merge it?

[janpio]

Please leave an "Approve" review on the PR @terpro.

In general someone has to come, understand and read through your PR @PieterVanPoyer, test and approve it, then merge. Later someone has to create a release of the plugin.

[terreng]

Working as expected on Android 5, 7, 8, 9

[PieterVanPoyer]

Jow,
It's been approved and tested by terpro!
Who can merge this, and make a release of this?

Some Github users did already fork my repo, to make use of this fix.

Kind regards,
Pieter

[millionfriends]

@janpio Hello, is there a chance that this pull request will be merged? That would be awesome as it's a really annoying bug that will be fixed by this PR. Thanks in advance!

[ghost]

+1 that this PR should be merged!

[satoshionoda]

any update about this PR?

[davidpadych]

knock knock ... Can you tell when someone gets to it and approve it?

[JamboBuenna]

+1 on please can this PR be merged in.

[brodybits]

I suggest you guys send this kind of request to dev@cordova.apache.org

…

On Mon, Apr 1, 2019, 6:00 AM JamboBuenna ***@***.***> wrote: +1 on please can this PR be merged in. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#74 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABfNUOp3tLrGEjgjvvTdzY4NuzEqGUveks5vcdi2gaJpZM4W7f-L> .

[rricamar]

Since the PR has been merged, does anybody know if its going to be a release in the next days/weeks? 🙏

[brodybits]

Since the PR has been merged, does anybody know if its going to be a release in the next days/weeks?

I recommend you send this kind of a request to: dev@cordova.apache.org

And keep in mind that Cordova is run by a bunch of hard-working volunteers.

[rricamar]

Since the PR has been merged, does anybody know if its going to be a release in the next days/weeks?

I recommend you send this kind of a request to: dev@cordova.apache.org

And keep in mind that Cordova is run by a bunch of hard-working volunteers.

Thanks @brodybits for the reply and the cordova community for your incredible work 👏