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

bug: 'onConnect' does not work as expected #110

Closed
2 of 3 tasks
UesleyCarossi opened this issue May 27, 2020 · 10 comments · Fixed by #114
Closed
2 of 3 tasks

bug: 'onConnect' does not work as expected #110

UesleyCarossi opened this issue May 27, 2020 · 10 comments · Fixed by #114

Comments

@UesleyCarossi
Copy link

UesleyCarossi commented May 27, 2020

Bug Report

Problem

I need to run an event when connecting to wifi, but it ends up running at unexpected times.

What is expected to happen?

The expected is to occur only when connecting to the internet.

What does actually happen?

The onConnect subscription is firing when the application is opened, minimized> open, when connected to the internet.

Information

Open the application, minimize and open that the onConnect event will be triggered.

Command or Code

https://github.com/UesleyCarossi/test-network

Environment, Platform, Device

Version information

Ionic:

   Ionic CLI                     : 5.4.1 (C:\Users\****\AppData\Roaming\npm\node_modules\ionic)
   Ionic Framework               : @ionic/angular 5.1.1
   @angular-devkit/build-angular : 0.901.7
   @angular-devkit/schematics    : 9.1.7
   @angular/cli                  : 9.1.7
   @ionic/angular-toolkit        : 2.2.0

Cordova:

   Cordova CLI       : 9.0.0 (cordova-lib@9.0.1)
   Cordova Platforms : android 8.0.0
   Cordova Plugins   : cordova-plugin-ionic-keyboard 2.2.0, cordova-plugin-ionic-webview 4.2.1, (and 5 other plugins)

Utility:

   cordova-res : 0.6.0
   native-run  : 0.2.8

System:

   Android SDK Tools : 26.1.1 (C:\Android\sdk)
   NodeJS            : v10.16.0 (C:\Program Files\nodejs\node.exe)
   npm               : 6.9.0
   OS                : Windows 10

Plugins:

cordova-plugin-device 2.0.2 "Device" cordova-plugin-ionic-keyboard 2.2.0 "cordova-plugin-ionic-keyboard" cordova-plugin-ionic-webview 4.2.1 "cordova-plugin-ionic-webview" cordova-plugin-network-information 2.0.2 "Network Information" cordova-plugin-splashscreen 5.0.2 "Splashscreen" cordova-plugin-statusbar 2.4.2 "StatusBar" cordova-plugin-whitelist 1.3.3 "Whitelist"

Checklist

  • I searched for existing GitHub issues
  • I updated all Cordova tooling to most recent version
  • I included all the necessary information above
@breautek
Copy link
Contributor

Which platforms are you experiencing this on?

@UesleyCarossi
Copy link
Author

Which platforms are you experiencing this on?

Tested only on Android

@breautek
Copy link
Contributor

breautek commented May 27, 2020

I've reproduced the described behaviour on an Android API 29 simulator using the latest cordova tooling and plugin.

Just posting some information... Couple of different theories.

Theory 1

Broadcast event is firing because we unregister and register to the native listener onPause/onResume. I assume the act of registering the event causes the event to fire at least once, producing an event to the javascript.

It didn't always unattach/re-attach the listener on pause/resume. This was changed by #74 for #64

Theory 2

The native broadcast simply fires when the activity is resumed.

I'm not really sure on the behaviour of the Intent broadcaster, but if theory 2 is actually what is happening, then I don't think this is a bug.

But if I were to make a bet, I think Theory 1 is why we are seeing these events on pause/resume. If we consider this a bug, then I think we should keep track of connection types, and only propagate the event to the javascript if the connection type actually changes. I'd support a PR that makes this change.

I'd also like to see if this behaviour is observable on iOS, but I don't have mac equipment to use to test this.

@UesleyCarossi
Copy link
Author

The problem is that the plugin behaves differently, based on the day it was cloned and executed npm i, cordova platform add android@8.0.0, cordova build android.

A few weeks ago we were looking for some way to identify when the user connected to wifi, we tested and confirmed that the cordova-plugin-network-information plugin would perfectly fulfill this purpose, so analysis and implementations were made in the project, but when we went integrating with the plugin its behavior was different from what we had tested weeks ago, we have 2 apks to prove it.

@UesleyCarossi
Copy link
Author

I downgraded from version 2.0.2 to 2.0.1 and it works perfectly. 😉

@breautek
Copy link
Contributor

breautek commented May 28, 2020

I downgraded from version 2.0.2 to 2.0.1 and it works perfectly.

well that's definitely interesting. I don't have time to poke around right now, but having a list of change commits between 2.0.1 and 2.0.2 would be helpful. Sounds like there was a regression that was introduced.

@UesleyCarossi
Copy link
Author

UesleyCarossi commented May 28, 2020

Maybe it's helpful, I use cordova-plugin-network-information in conjunction with cordova-plugin-background-mode.

@PieterVanPoyer
Copy link
Contributor

I've reproduced the described behaviour on an Android API 29 simulator using the latest cordova tooling and plugin.

Just posting some information... Couple of different theories.

Theory 1

Broadcast event is firing because we unregister and register to the native listener onPause/onResume. I assume the act of registering the event causes the event to fire at least once, producing an event to the javascript.

It didn't always unattach/re-attach the listener on pause/resume. This was changed by #74 for #64

Theory 2

The native broadcast simply fires when the activity is resumed.

I'm not really sure on the behaviour of the Intent broadcaster, but if theory 2 is actually what is happening, then I don't think this is a bug.

But if I were to make a bet, I think Theory 1 is why we are seeing these events on pause/resume. If we consider this a bug, then I think we should keep track of connection types, and only propagate the event to the javascript if the connection type actually changes. I'd support a PR that makes this change.

I'd also like to see if this behaviour is observable on iOS, but I don't have mac equipment to use to test this.

Hi

Thanks for the great work with Cordova!

I think Theory 1 is correct.
By reattatching the native listener it fires.
This reattaching helped to fix the bug where the connection state seems to be wrong after an amount of time when the app was idle.

I admit, it would be an improvement if we could keep the state of te connection and only propagate a change when the online/offline state effectivily changes.

In my opinion, this plugin is very usefull, but aims to do too much.
It was originally developed when there we're a lot less restrictions and rules in the Android system.

  • Now the Android OS can stop the listeners for working when the app is idle.
  • Now the Android OS is much more watching the battery drain, and stopping apps when they are idle.
  • Now there are a lot of more restrictions to privacy, and this affects the detection of the effective Network.
  • The ios system has even more restrictions about privacy.

In the documentation of this plugin there is an example of an upload of a file in the background https://github.com/apache/cordova-plugin-network-information#sample-upload-a-file-depending-on-your-network-state- , this used to work great, but now you have to work with WorkManager and stuff like that to do work in the background.

A suggestion

Loosen the requirements of the plugin to it's barebone's essentials.

Requirements

  • Detect online/offline only when the app is active.
  • When online, only detect following types: WIFI, MOBILE
  • If possible, only fire when the state effectivily changes. So keep track of the previous state before the app went in the background. And the app is not killed by the system.

Stop with next requirements

  • Detect online/offline when the app is not active.
  • Don't try to detect other network type's beside WIFI and MOBILE.
  • Do not let this plugin support background work like fileupload's and datasyncs. Remove it from the doc's.

A suggestion to do on short notice

Kind regards
Pieter

@PieterVanPoyer
Copy link
Contributor

PieterVanPoyer commented Jun 6, 2020

Hey

I did check the Android code and found something interesting.

On next lines, there is a check on the equality of the network state before and after a change.
The code seems to try to send an update only after a change. But this code goes long way back.

JSONObject thisInfo = this.getConnectionInfo(info);
if(!thisInfo.equals(lastInfo))
{
String connectionType = "";
try {
connectionType = thisInfo.get("type").toString();
} catch (JSONException e) {
LOG.d(LOG_TAG, e.getLocalizedMessage());
}
sendUpdate(connectionType);
lastInfo = thisInfo;
}

The thisInfo and lastInfo are JSONObjects.

But when I run following code in a testproject. 2 JSONObjects with the same values are not seen as equal.

try {
            JSONObject connectionInfo = new JSONObject();
            connectionInfo.put("type", "myType");
            connectionInfo.put("extraInfo", "myExtraInfo");

            JSONObject otherConnectionInfo = new JSONObject();
            otherConnectionInfo.put("type", "myType");
            otherConnectionInfo.put("extraInfo", "myExtraInfo");

            prependToNetworkOutputState(this.tvNetworkStateOutputKeepsListening, "equals JSONObjects " + connectionInfo.equals(otherConnectionInfo)); // THIS EQUALS RETURNS FALSE
        } catch (JSONException e) {

        }

I found something like this on StackOverflow: the Android's built-in JSONObject does not define hashCode() and equals() in any sensible way.
Check also the Android Docs: Android Docs

Can anyone confirm this?
If yes, then we should just correct the equality check.

There is also another path we have to check for state, but that's for later.

Kind regards,
Pieter

@breautek breautek added this to the 3.0.0 milestone Jun 6, 2020
breautek pushed a commit that referenced this issue Jul 8, 2020
…rmation#110 (#114)

* Try to fix #110 - The android JSONObject doesn't provide an equal implementation for JSON-Object.

* undo autoreformatting of android studio

* Android - remove JSONObject and extraInfo. KIS.

* Update src/android/NetworkManager.java

Co-authored-by: Tim Brust <ratchet.player@gmx.de>

* Update src/android/NetworkManager.java

Co-authored-by: Tim Brust <ratchet.player@gmx.de>

* Update src/android/NetworkManager.java

Co-authored-by: Tim Brust <ratchet.player@gmx.de>

* Android - code review PR

* Android - code review PR -> remove obsolete logging of constant WIFI

Co-authored-by: Tim Brust <ratchet.player@gmx.de>
@gidhin
Copy link

gidhin commented Jan 19, 2022

Any update on this issue. I'm facing the same in 3.0.0 in ANDROID.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants