Android - Fixes https://github.com/apache/cordova-plugin-network-information#110

PieterVanPoyer commented 4 years ago

Platforms affected

Android

Motivation and Context

Fixes https://github.com/apache/cordova-plugin-network-information/issues/110 .

Description

The JSONObject of Android does not have an equals implementation. So the equals between previous networkinfo and new networkinfo state always returned false. I've explicitly tested all properties of the JSONObject for equality.

Testing

I did test it on an Android Nexus 5X Emulator.

Checklist

[x] I've run the tests to see all new and existing tests pass
[ ] I added automated test coverage as appropriate for this change
[ ] 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)
[ ] I've updated the documentation if necessary

I didn't prefix the commit, but I did prefix the PR.

PieterVanPoyer commented 4 years ago

Hey

Can someone review this, please. During the startup procedure, there are also events dispatched, but they are not checked with the equals mechanism (Same as before). But IMO sometimes it fires 2 times when starting up the app. I think that's more a job for a Cordova specialist, or we could wait for an issue to be filed.

In the startup routine it is just fired, no check has been done. https://github.com/apache/cordova-plugin-network-information/blob/9f8527062776ead04982d7fab12875a66bf69855/src/android/NetworkManager.java#L114-L127

Maybe we should change the documentation and mention that no checks on Android are done when the app is in the background/idle.

Kind regards Pieter

breautek commented 4 years ago

Thank you for your PR :bow:

This looks good to me. While this is fixing a bug, it is changing behaviour (which is to stop event firing on resume, if there was no change in state), which is a behaviour that some people may be dependent on. Because of this, I think we need to treat this as a breaking change.

Currently the plugin is targeting a patch release (2.0.3-dev) so it may be some time before this actually gets merged. I noticed that the PR is sourced from your fork's master branch. I think you should move the 2 commits into a new branch and recreate this PR. (You can literally copy & paste your description). This is to avoid having accidental commits pushed to this PR.

When you do this I'll give my explicit approval.

Testing notes

I've tested in an API 29 emulator and went through the following paths:

Path 1

Put app into background
Put app into foreground

Saw no event fire as expected, because there was no network change. :heavy_check_mark:

Path 2

App in foreground.
Turned off wifi
Observed "4g" event
Turned on wifi.
Observed "wifi" event.

Events fired as expected. :heavy_check_mark:

Path 3

App in foreground
App in background
Turned off wifi
App in foreground

"4g" event fired as expected on resume. :heavy_check_mark:

PieterVanPoyer commented 4 years ago

I've made a new PR, starting from another branche. This one may be closed.

breautek commented 4 years ago

Thanks. The new PR that pulls from a sourced branch is located at https://github.com/apache/cordova-plugin-network-information/pull/114.

apache / cordova-plugin-network-information