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.

Sign up for GitHub

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

Jump to bottom

Fix popup for reconnecting to the server #4358

Closed
wants to merge 5 commits into from
Closed

Fix popup for reconnecting to the server #4358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
faccb1e
Fix popup for reconnecting to the server
Roffild Apr 21, 2024
cd4fe6b
ktlintCheck
Roffild Apr 21, 2024
e1c7493
Merge branch 'master' into fix_popup
Roffild May 7, 2024
1f90f95
onResume() fix
Roffild May 8, 2024
332ed4c
Merge branch 'master' into fix_popup
Roffild Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/src/main/java/io/homeassistant/companion/android/webview/WebView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ interface WebView {
TIMEOUT
}

fun loadUrl(url: String, keepHistory: Boolean, openInApp: Boolean)
fun loadUrl(url: String, keepHistory: Boolean = false, openInApp: Boolean = true)

fun setStatusBarAndNavigationBarColor(statusBarColor: Int, navigationBarColor: Int)

Expand Down
60 changes: 52 additions & 8 deletions app/src/main/java/io/homeassistant/companion/android/webview/WebViewActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import android.content.Intent
import android.content.pm.ActivityInfo
import android.content.pm.PackageManager
import android.content.res.Configuration
import android.graphics.Bitmap
import android.graphics.Color
import android.graphics.Rect
import android.net.Uri
Expand Down Expand Up @@ -39,6 +40,7 @@ import android.webkit.SslErrorHandler
import android.webkit.URLUtil
import android.webkit.ValueCallback
import android.webkit.WebChromeClient
import android.webkit.WebResourceError
import android.webkit.WebResourceRequest
import android.webkit.WebResourceResponse
import android.webkit.WebView
Expand Down Expand Up @@ -112,7 +114,6 @@ import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.Job
import kotlinx.coroutines.delay
import kotlinx.coroutines.launch
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.withContext
import okhttp3.HttpUrl.Companion.toHttpUrl
Expand Down Expand Up @@ -208,6 +209,8 @@ class WebViewActivity : BaseActivity(), io.homeassistant.companion.android.webvi
private var mFilePathCallback: ValueCallback<Array<Uri>>? = null
private var isConnected = false
private var isShowingError = false
private var isPageFinished = false
private var pageError = 0 // 0 - OK, <0 - Connection Error, >=400 - HTTP Error
Copy link
Member

Choose a reason for hiding this comment

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

What purpose does this have? It is not used in the UI? I can only see it in a log statement when reloading at which point it is no longer relevant because there is reloading 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used in reloadPage. It is possible to show more information about the error to the user.

Copy link
Member

Choose a reason for hiding this comment

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

But you're not showing anything. For debugging with http status codes etc. you may as well use Chrome remote debugging, the app is never going to match that level of detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently used for status in WebView.
It is now possible to show the tip.
pageError < 0:

Check your network connection.

pageError >= 400 (Android 6.0):

Check the server configuration.

private var isRelaunching = false
private var alertDialog: AlertDialog? = null
private var isVideoFullScreen = false
Expand Down Expand Up @@ -329,6 +332,15 @@ class WebViewActivity : BaseActivity(), io.homeassistant.companion.android.webvi
settings.mediaPlaybackRequiresUserGesture = !presenter.isAutoPlayVideoEnabled()
settings.userAgentString = settings.userAgentString + " ${HomeAssistantApis.USER_AGENT_STRING}"
webViewClient = object : TLSWebViewClient(keyChainRepository) {
override fun onPageStarted(view: WebView?, url: String?, favicon: Bitmap?) {
// onReceivedHttpError is getting called before onPageStarted.
// https://issuetracker.google.com/issues/210920403
// onPageStarted is always called before onPageFinished.
// For localhost it is possible to catch onReceived* after onPageFinished !
isPageFinished = false
// Don't pageError=0 here!
jpelgrom marked this conversation as resolved.
Show resolved Hide resolved
}

@Deprecated("Deprecated in Java for SDK >= 23")
override fun onReceivedError(
view: WebView?,
Expand All @@ -338,11 +350,22 @@ class WebViewActivity : BaseActivity(), io.homeassistant.companion.android.webvi
) {
Log.e(TAG, "onReceivedError: errorCode: $errorCode url:$failingUrl")
if (failingUrl == loadedUrl) {
pageError = errorCode
showError()
}
}

@RequiresApi(Build.VERSION_CODES.M)
override fun onReceivedError(view: WebView?, request: WebResourceRequest?, error: WebResourceError?) {
Log.e(TAG, "onReceivedError: errorCode: ${error?.errorCode} ${error?.description} url:${request?.url}")
Copy link
Member

Choose a reason for hiding this comment

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

To prevent flooding the logs, could this be moved inside the if() block below? We're not going to do anything with it otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a copy-paste as in line 351. There is no log flooding here.

Copy link
Member

Choose a reason for hiding this comment

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

The existing code isn't perfect. I think my comment about this being an unnecessary log statement when nothing is done with it still stands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. But this is not my code:
https://github.com/home-assistant/android/blob/2024.5.2/app/src/main/java/io/homeassistant/companion/android/webview/WebViewActivity.kt#L339-L342
https://github.com/home-assistant/android/blob/2024.5.2/app/src/main/java/io/homeassistant/companion/android/webview/WebViewActivity.kt#L373-L376

Replace?

if (request?.isForMainFrame == true && request.url.toString() == loadedUrl) {
pageError = error?.errorCode ?: -1
showError()
}
}

override fun onPageFinished(view: WebView?, url: String?) {
isPageFinished = true
if (clearHistory) {
webView.clearHistory()
clearHistory = false
Expand Down Expand Up @@ -371,7 +394,8 @@ class WebViewActivity : BaseActivity(), io.homeassistant.companion.android.webvi
errorResponse: WebResourceResponse?
) {
Log.e(TAG, "onReceivedHttpError: ${errorResponse?.statusCode} : ${errorResponse?.reasonPhrase} for: ${request?.url}")
if (request?.url.toString() == loadedUrl) {
if (request?.isForMainFrame == true && request.url.toString() == loadedUrl) {
pageError = errorResponse?.statusCode ?: 404
Copy link
Member

Choose a reason for hiding this comment

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

Why fallback to 404? HTTP errors can be for various codes; if unknown is a valid response the pageError variable should be nullable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pageError cannot be 0 (for an error) or null.
onReceivedHttpError always returns >=400.
This 404 is for Kotlin.

showError()
}
}
Expand Down Expand Up @@ -903,6 +927,10 @@ class WebViewActivity : BaseActivity(), io.homeassistant.companion.android.webvi
checkAndWarnForDisabledLocation()
}
changeLog.showChangeLog(this, false)

if (::loadedUrl.isInitialized) {
waitForConnection()
}
}

override fun onStop() {
Expand Down Expand Up @@ -1225,7 +1253,10 @@ class WebViewActivity : BaseActivity(), io.homeassistant.companion.android.webvi
}

override fun loadUrl(url: String, keepHistory: Boolean, openInApp: Boolean) {
Log.d(TAG, "loadUrl(url=$url, keepHistory=$keepHistory, openInApp=$openInApp)")
if (openInApp) {
isPageFinished = false
pageError = 0
loadedUrl = url
clearHistory = !keepHistory
webView.loadUrl(url)
Expand Down Expand Up @@ -1394,27 +1425,29 @@ class WebViewActivity : BaseActivity(), io.homeassistant.companion.android.webvi
alert.setPositiveButton(commonR.string.settings) { _, _ ->
startActivity(SettingsActivity.newInstance(this))
}
val isInternal = serverManager.getServer(presenter.getActiveServer())?.connection?.isInternal() == true
val connection = serverManager.getServer(presenter.getActiveServer())?.connection
val isInternal = connection?.isInternal() == true
alert.setNegativeButton(
if (failedConnection == "external" && isInternal) {
commonR.string.refresh_internal
} else {
commonR.string.refresh_external
}
) { _, _ ->
runBlocking {
if (connection != null) {
failedConnection = if (failedConnection == "external") {
serverManager.getServer(presenter.getActiveServer())?.let { webView.loadUrl(it.connection.getUrl(true).toString()) }
loadUrl(connection.getUrl(true).toString())
"internal"
} else {
serverManager.getServer(presenter.getActiveServer())?.let { webView.loadUrl(it.connection.getUrl(false).toString()) }
loadUrl(connection.getUrl(false).toString())
"external"
}
} else {
waitForConnection()
}
waitForConnection()
}
alert.setNeutralButton(commonR.string.wait) { _, _ ->
waitForConnection()
reloadPage()
Copy link
Member

Choose a reason for hiding this comment

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

Please revert. The button says Wait - it is for when loading takes >10s (which triggers this pop-up) and you want to wait/see it's almost done. It should not reload, that is what the reload button is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reloadPage reloads when there is an ERROR + run waitForConnection. Only waitForConnection does not work on error.

Copy link
Member

Choose a reason for hiding this comment

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

The wait button is not for errors. In case of an error, you are supposed to fix it and refresh :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the WebView has an ERROR, then WAIT doesn't work.
Details below...

Copy link
Member

@jpelgrom jpelgrom May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, but this still doesn't address the contradiction you've created by making the wait button refresh. I've submitted an alternative in #4412 as I'm tired of this discussion.

}
}
alertDialog = alert.create()
Expand Down Expand Up @@ -1511,6 +1544,7 @@ class WebViewActivity : BaseActivity(), io.homeassistant.companion.android.webvi
}

private fun waitForConnection() {
// Called inside an this.loadUrl() [not webView.loadUrl()]
Copy link
Member

Choose a reason for hiding this comment

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

This is called from other functions as well? I don't really understand the comment and there seems to be broken Markdown syntax as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not Markdown. This is a pointer to line 1263.

Copy link
Member

@jpelgrom jpelgrom May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Syntax aside, this is still called from other functions as well and I don't really see what the comment adds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a note:

// BAD:
this.loadUrl()
this.waitForConnection()

// Good:
this.loadUrl()

// Good:
webView.loadUrl()
this.waitForConnection()

Handler(Looper.getMainLooper()).postDelayed(
{
if (
Expand All @@ -1525,6 +1559,16 @@ class WebViewActivity : BaseActivity(), io.homeassistant.companion.android.webvi
)
}

private fun reloadPage(force: Boolean = false) {
if (force || isPageFinished || pageError != 0) {
Log.d(TAG, "reloadPage: isPageFinished=$isPageFinished pageError=$pageError")
isPageFinished = false
pageError = 0
webView.reload()
}
waitForConnection()
}

override fun sendExternalBusMessage(message: ExternalBusMessage) {
val map = mutableMapOf(
"id" to message.id,
Expand Down