Fix popup for reconnecting to the server #4358
Conversation
There was a problem hiding this comment.
Demanding merges via Discord is very much not appreciated.
The relation to the issue (which in itself is hard to follow) is unclear. I'm seeing some attempts to handle errors in another callback here and trying to review those, but a clear issue description would be appreciated.
| } | ||
| alert.setNeutralButton(commonR.string.wait) { _, _ -> | ||
| waitForConnection() | ||
| reloadPage() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
reloadPage reloads when there is an ERROR + run waitForConnection. Only waitForConnection does not work on error.
There was a problem hiding this comment.
The wait button is not for errors. In case of an error, you are supposed to fix it and refresh :)
app/src/main/java/io/homeassistant/companion/android/webview/WebViewActivity.kt
Show resolved
Hide resolved
|
|
||
| @RequiresApi(23) | ||
| override fun onReceivedError(view: WebView?, request: WebResourceRequest?, error: WebResourceError?) { | ||
| Log.e(TAG, "onReceivedError: errorCode: ${error?.errorCode} ${error?.description} url:${request?.url}") |
There was a problem hiding this comment.
To prevent flooding the logs, could this be moved inside the if() block below? We're not going to do anything with it otherwise.
There was a problem hiding this comment.
This is a copy-paste as in line 351. There is no log flooding here.
There was a problem hiding this comment.
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.
| @@ -470,7 +494,7 @@ class WebViewActivity : BaseActivity(), io.homeassistant.companion.android.webvi | |||
| startActivity(browserIntent) | |||
| return true | |||
| } else { | |||
| Log.d(TAG, "No unique cases found to override") | |||
| // Log.d(TAG, "No unique cases found to override url=${it}") // Unstoppable flood. | |||
There was a problem hiding this comment.
Please revert. We don't leave commented code like this, and if you're seeing this statement a lot that is probably an issue with your setup (it is only called for main page navigation/redirects, so with a normal setup you don't see it unless trying to navigate to outside). Some of the log statements you added will generate more lines than this one.
There was a problem hiding this comment.
Without a URL, the information content of this log is poor.
This log shows the correct URL, which has no additional action.
For example, 3 logs per second: https://community.home-assistant.io/t/apps-cant-access-ha/534210/3
I was receiving more than 10 logs per second.
There was a problem hiding this comment.
Adding an url could be useful but you're commenting it out so in the end adding no value. And as mentioned, we don't leave in commented code like this.
I was receiving more than 10 logs per second.
Did you manage to find out what caused it? If so, that could be discussed in an issue and/or changed/fixed in a different PR.
| @@ -1511,6 +1544,7 @@ class WebViewActivity : BaseActivity(), io.homeassistant.companion.android.webvi | |||
| } | |||
|
|
|||
| private fun waitForConnection() { | |||
| // Called inside an this.loadUrl() [not webView.loadUrl()] | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is not Markdown. This is a pointer to line 1263.
There was a problem hiding this comment.
Syntax aside, this is still called from other functions as well and I don't really see what the comment adds.
| @@ -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 | |||
There was a problem hiding this comment.
Why fallback to 404? HTTP errors can be for various codes; if unknown is a valid response the pageError variable should be nullable.
There was a problem hiding this comment.
pageError cannot be 0 (for an error) or null.
onReceivedHttpError always returns >=400.
This 404 is for Kotlin.
| @@ -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 | |||
There was a problem hiding this comment.
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 🙃
There was a problem hiding this comment.
Used in reloadPage. It is possible to show more information about the error to the user.
There was a problem hiding this comment.
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.
|
|
||
| if (::loadedUrl.isInitialized) { | ||
| reloadPage() | ||
| } |
There was a problem hiding this comment.
Please revert. This will cause a reload with state loss anytime the app is resumed, eg. when opening it from the background.
There was a problem hiding this comment.
(Fixed) It's better to use waitForConnection here.
This is a test after returning from Settings.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
This still doesn't describe the issue (when down an error is expected, and the refresh button refreshes) and also doesn't match the code changes which are all over the place. Trying hard to find something here but if we cannot agree on an issue and the value of changes, maybe this should be rejected. |
After receiving an ERROR, the page must be reloaded.
WebView cannot report an error from the server.
Closed #4272