delight-im / Android-DDP

[UNMAINTAINED] Meteor's Distributed Data Protocol (DDP) for clients on Android
Apache License 2.0
274 stars 54 forks source link

Network monitoring and Login with token #87

Closed daupawar closed 8 years ago

daupawar commented 8 years ago

in my app i need functionality for auto login as i forked project and made login with token public and also whenever i call Meteor.call or meteor.update i am getting warning "too much task on main thread" changes made in this forked project https://github.com/daupawar/Android-DDP 1) added functionality for network monitoring-line 231 2) call method in new thread (asyctask)-341 (for network call on main thread issue) 3) loginwithtoken method made public- line number 883 Note-: setWORKLOCAL false will ignore network monitoring in method call

ocram commented 8 years ago

Thanks a lot for contributing your changes!

I have a few questions:

  1. What does your network monitoring do exactly? Does it just reject operations when the device has no internet connection? Is this really required (and desired)? What's the current situation? Does your app crash without this?
  2. It seems you've wrapped the workload of Meteor#send in an AsyncTask, right? (We could also use a simple Thread.) Do you really get The application may be doing too much work on its main thread otherwise? Have others been experiencing this as well?
  3. Why do you need loginWithToken to be public? Does this related issue help?
daupawar commented 8 years ago

A)

  1. Network monitoring code monitors current state of network status. and aslo it has listener onInternetStatusChanged.
  2. this code not reject the operations it fires onError with reason , please check isValidToCall() method for more info.
  3. In my app i really need network monitoring , if i not do so then i need check network status every button click.
  4. i also added flag setWORKLOCAL if someone don't want to use network monitoring

B)

  1. my application consumes huge JSON data, in that case i am getting warning on log The application may be doing too much work on its main thread, i don't think everyone is getting same warning but after wrapping Meteor#send in AsynckTask, i am not getting this warning,
  2. suggest me best way to do so (AsynckTask or Thread) C)
  3. In my app i need to implement auto-login functionality, so i use previously stored token for auto-login
ocram commented 8 years ago

Network connectivity ("A"): You really shouldn't have to check internet connectivity yourself, even less so on every click of a button. So we 100% agree here.

But I'm not sure if network monitoring is really required for this. As you can see here, this library should queue all JSON messages when they can't be sent over the WebSocket. This should work today, already.

If it doesn't work, we should fix it. What's wrong for you right now? Let's track this problem here: https://github.com/delight-im/Android-DDP/issues/89

Too much work on main thread ("B"): We should definitely implement your fix for this problem. Thank you!

You've already tracked down the correct method that should be wrapped in a separate thread. But is it the toJson call or the send call that's so costly? Did you test this, or can you check what's consuming more time and resources?

Public access to token-based login ("C"): This library does already handle auto-login, doesn't it? If you sign in using email/username and password, the library should receive a token upon successful login, store it, and try to sign in with it the next time. Doesn't this work? Or do you need token-based login for your own social sign-in? In the latter case, does this issue help?

daupawar commented 8 years ago

Network connectivity library queue all json messages Its ok, but app user cant understand whats going on. it will be good if we can show app user the proper message "no connection etc". case 1: public void call(final String methodName, final ResultListener listener) if i call this method when no internet connection and i want to do something onSuccess or onError, how can i handle this case? How can i give user proper error message?

Too much work on main thread I will test what's consuming more time and resources.

Public access to token-based login My app working flow is like this. 1) I handle autologin on splash screen, if token is valid app redirects to main screen else it shows login screen , i dont want to ask username password to user every time 2) i use loginwithtoken method when user come from notification

ocram commented 8 years ago

Network connectivity: The callbacks onSuccess or onError of your ResultListener should still be called for queued messages. That is, whenever they're released from the queue and successfully sent to the server. So, unless there's a bug here, everything should work conveniently and as expected.

I agree that the user may be confused why an operation is taking so long when it couldn't be dispatched but had to be queued instead. But isn't onDisconnect usually called in that case? If not, we should fix it. In this specific case, we could even replace onDisconnect with a new callback such as onConnectionLost or onWaitingForConnection.

But apart from that, everything should work fine already today. So can we discuss the problems of bad network connectivity again, please? There should be nothing that you need to add in order to make this library work when connections are lost and re-established. If something doesn't work, it's probably a bug and we should fix it.

Too much work on main thread: Great, thank you! We'll just run that one call in a separate thread.

Public access to token-based login: Where do you get that token from that you're checking for validity? Do you extract it from this library by exposing read access to the token as well?

Why can't you just connect with this library on your splash screen, wait for onConnect to be called, and use its parameter signedInAutomatically to determine if the token was valid or not? If the token was valid, you would be signed-in already.

daupawar commented 8 years ago

Public access to token-based login: signedInAutomatically resolved my problem

Too much work on main thread: I cant figure out actual line of code where i get main thread warning so for now we can ignore the warning.

Network connectivity: This is still open issue for me onDisconnect called when i disconnect internet, but onDisconnect not called again if already disconnected and user fires call again i cant handle this case,

ocram commented 8 years ago

Thanks! Great that we have resolved one of the issues, now let's solve the other two problems as well.

Too much work on main thread: Does that mean that you're not sure if the problem is still there? So we don't have to change anything? Or is it just that you can't determine if we have to put the call to toJson or the call to send within Meteor#send in a separate thread?

Network connectivity: If I understand you correctly, the library fires your callback onDisconnect whenever the internet connection is lost. This is what is expected and thus it's fine.

Now as we discussed already, when internet connection is not available and you try to send a message, it's queued, which is fine as well. But in this case, you expect the library to fire onDisconnect again (because the message couldn't be sent), right? Is this what you were referring to?

That's actually not what should happen. You should only receive onDisconnect in the situations where it's fired already today. In the other case, when subsequent messages cannot be sent and need to be queued, there's no callback that should be fired, and I think that's what is being done. You may add an instance of ResultListener to your call, and then ResultListener#onSuccess or ResultListener#onError should be fired eventually, but only when the connection has been established again and a new attempt is made.

I think what you want could be an additional callback ResultListener#onWaiting or ResultListener#onQueued for a single call that tells you that a message has not been sent yet and there will be a new attempt shortly. Is this correct?

daupawar commented 8 years ago

Too much work on main thread: We don't have to change anything.

Network connectivity: Your #98 solution will work for same case.

rakirox commented 8 years ago

I will try to this by today :) https://github.com/delight-im/Android-DDP/issues/98

ocram commented 8 years ago

@daupawar So can we close this issue here and track the remaining problem (network connectivity) in https://github.com/delight-im/Android-DDP/issues/98 until it's done?

daupawar commented 8 years ago

@mwaclawek thanks for your continuous support.

derwaldgeist commented 5 years ago

I need the "login with token" functionality, too. Has this been implemented since?