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

Implemented Auto Refresh for WebView #77

Merged
merged 12 commits into from May 27, 2020
Merged

Implemented Auto Refresh for WebView #77

merged 12 commits into from May 27, 2020

Conversation

nizarmah
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

Instead of implementing swipe to refresh, the web view refreshes on its own. Here is how it works:

  1. The WebViewClient receives an error
  2. The client checks if the user is offline. If user is offline, an event is posted.
  3. Event starts a BroadcastReceiver, which checks if the user is back online on connectivity change.
  4. If the user is back online, an event is posted indicating the user is back online.
  5. The event is captured, and the WebView is reloaded and the BroadcastReceiver is stopped.

Please note that the view.reload() reposts POST parameters if the request is POST. If you want me to change that functionality, I can just view.loadUrl(view.url).

I took into consideration the work leaks. Everything was unregistered (BroadcastReceiver) or canceled (Coroutines). Also, nothing runs on the main thread, except the view.reload because it is interacting with the WebView.

It is important to note that the webview client is not automatically registered to the Event. This is because we want to avoid leaks. Therefore, whenever it is manually registered, it should be manually unregistered.

Related Tickets & Documents

This resolves issue #76

Screenshots/Recordings (if there are UI changes)

There are no UI changes.

[optional] What gif best describes this PR or how it makes you feel?

Rocky - Celebration

Updated Libs and changed strings in implementations to variables in Libs object
Updated Kotlin Version to v1.3.71 since Kotlin Coroutines v1.3.7 are only supported with Kotlin v1.3.7+
NetworkStatusEvent will be used with EventBus, it will carry the NetworkStatus to Listeners or Subscribers to that Event

Once the NetworkStatusEvent is received, certain actions will be taken, depending on if the device is online or offline.
Checking if user is online is done through opening a socket with 8.8.8.8 (google dns). This online check is according to the stackoverflow answer linked below. It is faster than other options.

https://stackoverflow.com/a/27312494/5512292
…line

This is used in order to check if the device is back online. Once the device's connectivity changes, ie is connected to internet after being offline, it posts an Event indicating the device is back online.
A coroutine scope is started in the main activity, in oder to prevent work leaks. onDestroy, all coroutines are cancelled.

The webview client takes the coroutine scope, again to avoid work leaks, and utilizes it to check if the user is offline when the webview client receives an error

Once the webview client receives an error, an Event indicating the user is offline is posted.

The webview client is subscribed to the NetworkStatusEvent in order to know the network status. Once the device is offline, the network watcher is started (a broadcast receiver waits until the device is back online). Once the device is back online, the network watcher is stopped and the webview is refreshed

It is important to note that the webview client is not automatically registered to the Event. This is because we want to avoid leaks. Therefore, whenever it is manually registered, it should be manually unregistered.
@CLAassistant
Copy link

CLAassistant commented May 25, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@fdocr fdocr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nizarmah thanks again for the PR! This seems like a thorough implementation, but I wasn't able to see it working locally.

I added a couple inline comments and here's a couple GIFs that show what I was doing to test this out. The GIFs turned out a little big so might have to let them load to see them well (~1 min long).

Without the Service Worker:
android-conn1

With the Service Worker:
android-conn2

@nizarmah
Copy link
Contributor Author

nizarmah commented May 25, 2020

I'm actually astonished that they aren't working at all for you. Here's a video of what's happening on my end.

I'll look further into it and let you know. Sorry about this inconvenience. Once I have figured out exactly what is wrong, I'll definitely update this PR. For now, I'll put it as [WIP] and change it back to draft.

@nizarmah nizarmah changed the title Implemented Auto Refresh for WebView [WIP] Implemented Auto Refresh for WebView May 25, 2020
@nizarmah nizarmah marked this pull request as draft May 25, 2020 18:32
@nizarmah
Copy link
Contributor Author

I found a possible issue. The refresh isn't working except once after launching the application. I need to look further in to it. I haven't done the proper testing that I should have, apparently. Sorry for that.

@pr-triage pr-triage bot added the PR: draft label May 25, 2020
Uses hostname instead of url, so created BuildConfig.baseHostname instead of trimming the 'https://' and '/' segments of the BuildConfig.baseUrl. This gives for easier changes later on in the future.

Also, @fdoxyz requested that change in order to reconnect to the website once it is up again, if it was down. Mentioned in pull request #77.
…Client

Previously, an exception was being thrown due to unregistering a network watcher and then trying to unregister it again later on. Now, since it is being cleared back to null, it is no longer being unregistered again if it was previously unregistered
@nizarmah nizarmah changed the title [WIP] Implemented Auto Refresh for WebView Implemented Auto Refresh for WebView May 25, 2020
@nizarmah nizarmah marked this pull request as ready for review May 25, 2020 19:30
@nizarmah nizarmah requested a review from fdocr May 25, 2020 19:30
Copy link
Contributor

@fdocr fdocr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fast reply @nizarmah, I ran a quick test and things are working for me now.

I left another very small inline comment as an optimization opportunity. Other than this I think the solution is working well :)

onReceiveError was being called multiple times, this was resulting in multiple events being posted indicating that the device is offline. Accordingly, it was important to stop re-registering the networkWatcher again and again whenever the events were captured. However, it was previously done in case the coroutineScope did change. However, if the coroutineScope changes, the activity has probably died and been recreated. Therefore, the webview would have already been destroyed and recreated with the new scope. Thus, there is no need to re-register a network watcher if it has already been registered
@nizarmah
Copy link
Contributor Author

Alright 🎉 I guess that's all! I'm glad you think the solution is working well!

I hope you liked my contribution, because I plan to contribute more to the DEV android application.
Kindly, let me know if there is anything you want me to change, whether in my commit messages or my syntax or coding style, so that the next contributions would be as you guys want them to be ( :

@nizarmah nizarmah requested a review from fdocr May 26, 2020 13:51
…bug/auto-refresh

# Conflicts:
#	app/build.gradle.kts
Copy link
Contributor

@fdocr fdocr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really appreciate all the work for this @nizarmah! And of course help on the repo is greatly appreciated! For feature requests maybe the best route to go with is opening an issue first but feel free to ping me anytime. I'll try to add some more small issues so we can have a clear path as to what to work on.

I just fixed a small conflict from another PR being merged.

@fdocr fdocr merged commit 9743b74 into forem:master May 27, 2020
@nizarmah
Copy link
Contributor Author

@fdoxyz yeah it would be great if there were labels indicating which issues can be worked on and which can't. I went through the issues earlier today and it was a bit of a mess. There were many things still open even though they were already merged. Meanwhile, there were other things that were open where I wasn't even able to recreate the bug. And there are also some issues that should be in the web app instead of the android application. I wasn't able to find any issue currently, out of the ones available, to start working on. If I missed one of them that can be done right now, before being implemented on the web app, please let me know.

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

Successfully merging this pull request may close these issues.

None yet

3 participants