Add check internet connectivity immediately #90

Kisty · 2016-10-07T15:48:34Z

This is so that the app checks for connection immediately instead of delaying the check. See http://reactivex.io/RxJava/javadoc/rx/Observable.html#interval(long,%20long,%20java.util.concurrent.TimeUnit)

See http://stackoverflow.com/a/3568906/818821

pwittchen · 2016-10-08T14:01:03Z

Hi, @Kisty. Thanks for your PR. It's a good idea to check Internet connectivity immediately. I'll test it as soon as I can. I'm quite busy now, so I'll take a closer look on that in the middle of the next week.

pwittchen · 2016-10-08T14:03:41Z

library/src/main/java/com/github/pwittchen/reactivenetwork/library/ReactiveNetwork.java

@@ -101,11 +101,26 @@ public static ReactiveNetwork create() {
   * and false if not
   */
  public static Observable<Boolean> observeInternetConnectivity() {
-    return observeInternetConnectivity(DEFAULT_PING_INTERVAL_IN_MS, DEFAULT_PING_HOST,
+    return observeInternetConnectivity(DEFAULT_PING_INTERVAL_IN_MS, DEFAULT_PING_INTERVAL_IN_MS, DEFAULT_PING_HOST,


You could create an additional private static final field called DEFAULT_INITIAL_PING_INTERVAL_IN_MS and use it here because it's related to a separate thing.

OK. Although the overloaded interval method that takes only delay and time unit maps to interval with initial delay and normal delay as the delay taken in the overloaded interval method, so I don't think it's needed.

I can put another constant in if you want though.

pwittchen · 2016-10-08T14:05:46Z

library/src/main/java/com/github/pwittchen/reactivenetwork/library/ReactiveNetwork.java

+                                                                final String host, final int port, final int timeoutInMs) {
+    if (initialIntervalInMs < 0) {
+      throw new IllegalArgumentException("initialIntervalInMs is not a non-zero number");
+    }


You could use Preconditions.checkPositive(int,String) method in the same way as it's used below. It does logically the same thing as this conditional statement.

Tried doing that but positive checking throws for 0 or lower so had to mirror the preconditions code.

And 0 is what we want for an immediate check.

pwittchen · 2016-10-08T14:06:33Z

library/src/main/java/com/github/pwittchen/reactivenetwork/library/ReactiveNetwork.java

-          }
-        })
-        .distinctUntilChanged();
+    return Observable.interval(initialIntervalInMs, intervalInMs, TimeUnit.MILLISECONDS, Schedulers.io())


Please, remember to update documentation in README.md file when you are updating library API.

pwittchen · 2016-10-08T14:07:30Z

The code is generally fine, but I had a few issues. :)

Kisty · 2016-10-08T16:26:01Z

Thanks, man. Also, did you want some tests? I wasn't sure how to do them or whi h class to do them in.

Kisty · 2016-10-11T11:44:14Z

Just making changes you've requested and pretty much ready to push. Shall we make the immediate check API as default in demo app and README examples or would you prefer to leave it not as default but add a note?

I think users may expect the check to be immediate, but that may just be me.

pwittchen · 2016-10-11T21:58:37Z

Thanks for updates. They look fine. I'll check it soon. If there won't be any problems, it'll be merged.

You asked about tests. Unit tests are always a good idea and they're more than welcome. In Android apps, unit testing is a bit tricky and harder than in regular Java app. You can take a look on existing (instrumentation) unit tests. New tests can be created in a similar way. To execute them, we need connected device or emulator. It's one of the available testing approaches on Android.

Kisty · 2016-10-12T06:18:12Z

Thanks for that. I can put some tests together to see if negative values throw exceptions for the method with initial interval as a parameter.

Thanks for explaining about the tests. I was trying to find code which test the expected behaviour of the observables, though am uncertain how to as the tests require internet or changing network types to assert the right network states are given in onNext. I'll have a look in to it.

pwittchen · 2016-10-12T21:46:55Z

Thanks for the updates. I've tested the code and it works fine.
Can you squash your commits into one? You can use this website: http://rebaseandsqua.sh/.
After the squash, I'm eager to merge. :)

Squashed commit of the following: commit b5d5dd8 Author: Timothy Kist <zboarda@gmail.com> Date: Wed Oct 12 14:51:15 2016 +0100 Improve valid observable tests commit 93de1cc Author: Timothy Kist <zboarda@gmail.com> Date: Wed Oct 12 14:26:32 2016 +0100 Fix default connectivity check commit 730b575 Author: Timothy Kist <zboarda@gmail.com> Date: Wed Oct 12 14:25:31 2016 +0100 Add tests for valid observables commit e93367d Author: Timothy Kist <zboarda@gmail.com> Date: Wed Oct 12 13:52:07 2016 +0100 Add tests for observing internet with initial interval commit 3c6b52a Author: Timothy Kist <zboarda@gmail.com> Date: Tue Oct 11 12:40:14 2016 +0100 Add documentation for checking internet connectivity immediately or specifying inital interval commit c18861e Author: Timothy Kist <zboarda@gmail.com> Date: Tue Oct 11 12:38:16 2016 +0100 Remove API documentation noise by removing redundant `final` keyword commit 22fd32f Author: Timothy Kist <zboarda@gmail.com> Date: Tue Oct 11 12:30:36 2016 +0100 Correct exception message commit fed5904 Author: Timothy Kist <zboarda@gmail.com> Date: Tue Oct 11 12:29:26 2016 +0100 Add default intial ping interval commit 91671ec Author: Tim Kist <timkist@imagitech.co.uk> Date: Fri Oct 7 16:42:53 2016 +0100 Add check internet connectivity immediately

Kisty · 2016-10-13T14:18:00Z

Thanks for pointing me to that script. I didn't run it but went through the commands trying to understand a bit of what it was doing. Was quite impressed with it. Will be using HEAD@{n} notation more often! Hope it looks good now.

Kisty · 2016-10-15T10:39:27Z

Hi! How does the PR look now?

pwittchen · 2016-10-15T11:18:16Z

It's fine now. I have some time today, so I'm merging it. Thanks @Kisty for your effort!

pwittchen · 2016-10-15T12:05:01Z

I'm planning to make immediate internet check introduced in this PR as default in PR #95 with a few other code cleanups. It will simplify library and we won't need an additional method for that. :-)

Fix crash when pre-lollipop receiver is already unregistered

f08aa74

See http://stackoverflow.com/a/3568906/818821

pwittchen self-assigned this Oct 8, 2016

pwittchen added the enhancement label Oct 8, 2016

pwittchen requested changes Oct 8, 2016

View reviewed changes

pwittchen approved these changes Oct 11, 2016

View reviewed changes

Kisty force-pushed the add_check_internet_connectivity_immediately branch from b5d5dd8 to b22b44e Compare October 13, 2016 14:15

pwittchen merged commit 154cac0 into pwittchen:master Oct 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check internet connectivity immediately #90

Add check internet connectivity immediately #90

Kisty commented Oct 7, 2016

pwittchen commented Oct 8, 2016

pwittchen Oct 8, 2016 •

edited

Kisty Oct 8, 2016

Kisty Oct 8, 2016

pwittchen Oct 8, 2016

Kisty Oct 8, 2016

Kisty Oct 8, 2016

pwittchen Oct 8, 2016

Kisty Oct 8, 2016

pwittchen commented Oct 8, 2016 •

edited

Kisty commented Oct 8, 2016

Kisty commented Oct 11, 2016

pwittchen commented Oct 11, 2016 •

edited

Kisty commented Oct 12, 2016

pwittchen commented Oct 12, 2016

Kisty commented Oct 13, 2016

Kisty commented Oct 15, 2016

pwittchen commented Oct 15, 2016

pwittchen commented Oct 15, 2016

Add check internet connectivity immediately #90

Add check internet connectivity immediately #90

Conversation

Kisty commented Oct 7, 2016

pwittchen commented Oct 8, 2016

pwittchen Oct 8, 2016 • edited

Choose a reason for hiding this comment

Kisty Oct 8, 2016

Choose a reason for hiding this comment

Kisty Oct 8, 2016

Choose a reason for hiding this comment

pwittchen Oct 8, 2016

Choose a reason for hiding this comment

Kisty Oct 8, 2016

Choose a reason for hiding this comment

Kisty Oct 8, 2016

Choose a reason for hiding this comment

pwittchen Oct 8, 2016

Choose a reason for hiding this comment

Kisty Oct 8, 2016

Choose a reason for hiding this comment

pwittchen commented Oct 8, 2016 • edited

Kisty commented Oct 8, 2016

Kisty commented Oct 11, 2016

pwittchen commented Oct 11, 2016 • edited

Kisty commented Oct 12, 2016

pwittchen commented Oct 12, 2016

Kisty commented Oct 13, 2016

Kisty commented Oct 15, 2016

pwittchen commented Oct 15, 2016

pwittchen commented Oct 15, 2016

pwittchen Oct 8, 2016 •

edited

pwittchen commented Oct 8, 2016 •

edited

pwittchen commented Oct 11, 2016 •

edited