You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@cordova.apache.org by GitBox <gi...@apache.org> on 2020/06/06 13:12:46 UTC
[GitHub] [cordova-plugin-network-information] PieterVanPoyer opened a new pull request #111: Android - Fixes https://github.com/apache/cordova-plugin-network-information#110
PieterVanPoyer opened a new pull request #111:
URL: https://github.com/apache/cordova-plugin-network-information/pull/111
<!--
Please make sure the checklist boxes are all checked before submitting the PR. The checklist is intended as a quick reference, for complete details please see our Contributor Guidelines:
http://cordova.apache.org/contribute/contribute_guidelines.html
Thanks!
-->
### Platforms affected
Android
### Motivation and Context
<!-- Why is this change required? What problem does it solve? -->
<!-- If it fixes an open issue, please link to the issue here. -->
Fixes the bug https://github.com/apache/cordova-plugin-network-information/issues/110 .
### Description
<!-- Describe your changes in detail -->
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
<!-- Please describe in detail how you tested your changes. -->
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](https://help.github.com/articles/closing-issues-using-keywords/))
- [ ] I've updated the documentation if necessary
I didn't prefix the commit, but I did prefix the PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-network-information] PieterVanPoyer edited a comment on pull request #111: Android - Fixes https://github.com/apache/cordova-plugin-network-information#110
Posted by GitBox <gi...@apache.org>.
PieterVanPoyer edited a comment on pull request #111:
URL: https://github.com/apache/cordova-plugin-network-information/pull/111#issuecomment-640059715
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-network-information] PieterVanPoyer commented on pull request #111: Android - Fixes https://github.com/apache/cordova-plugin-network-information#110
Posted by GitBox <gi...@apache.org>.
PieterVanPoyer commented on pull request #111:
URL: https://github.com/apache/cordova-plugin-network-information/pull/111#issuecomment-640085990
I've made a new PR, starting from another branche.
This one may be closed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-network-information] breautek closed pull request #111: Android - Fixes https://github.com/apache/cordova-plugin-network-information#110
Posted by GitBox <gi...@apache.org>.
breautek closed pull request #111:
URL: https://github.com/apache/cordova-plugin-network-information/pull/111
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-network-information] breautek commented on pull request #111: Android - Fixes https://github.com/apache/cordova-plugin-network-information#110
Posted by GitBox <gi...@apache.org>.
breautek commented on pull request #111:
URL: https://github.com/apache/cordova-plugin-network-information/pull/111#issuecomment-640088116
Thanks. The new PR that pulls from a sourced branch is located at https://github.com/apache/cordova-plugin-network-information/pull/114.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-network-information] PieterVanPoyer commented on pull request #111: Android - Fixes https://github.com/apache/cordova-plugin-network-information#110
Posted by GitBox <gi...@apache.org>.
PieterVanPoyer commented on pull request #111:
URL: https://github.com/apache/cordova-plugin-network-information/pull/111#issuecomment-640059715
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.
Kind regards
Pieter
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-network-information] breautek commented on pull request #111: Android - Fixes https://github.com/apache/cordova-plugin-network-information#110
Posted by GitBox <gi...@apache.org>.
breautek commented on pull request #111:
URL: https://github.com/apache/cordova-plugin-network-information/pull/111#issuecomment-640073128
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
1. Put app into background
2. Put app into foreground
Saw no event fire as expected, because there was no network change. :heavy_check_mark:
#### Path 2
1. App in foreground.
2. Turned off wifi
3. Observed "4g" event
4. Turned on wifi.
5. Observed "wifi" event.
Events fired as expected. :heavy_check_mark:
#### Path 3
1. App in foreground
2. App in background
3. Turned off wifi
4. App in foreground
"4g" event fired as expected on resume. :heavy_check_mark:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
[GitHub] [cordova-plugin-network-information] breautek closed pull request #111: Android - Fixes https://github.com/apache/cordova-plugin-network-information#110
Posted by GitBox <gi...@apache.org>.
breautek closed pull request #111:
URL: https://github.com/apache/cordova-plugin-network-information/pull/111
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org