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

Hide 'Wait' if WebViewActivity error is not due to external bus timeout #4412

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jpelgrom
Copy link
Member

@jpelgrom jpelgrom commented May 17, 2024

Summary

Hide the 'Wait' button in WebViewActivity errors if the error is not due to an external bus timeout, but a more general loading error where there's nothing to wait for. The PR also slightly updates what the refresh button does to ensure loading errors on refresh trigger immediately (instead of waiting for a timeout) + uses the URL it says on the button text.

This PR was inspired by the (previously unheard) confusion here, and looking back perhaps also by this suggested change in there (although that change wasn't explained, and incomplete).

Screenshots

You get a smaller dialog without a wait button:

Light Dark
Companion app with 'Unable to connect to Home Assistant' error, and only two buttons, which can now be displayed on one line instead of three, light mode Companion app with 'Unable to connect to Home Assistant' error, and only two buttons, which can now be displayed on one line instead of three, dark mode

Link to pull request in Documentation repository

n/a

Any other notes

 - The error pop up in WebViewActivity can show up for various reasons. The option to wait is only relevant when it is due to a timeout waiting on the external bus, otherwise it is an error for page loading/ssl/...  where waiting won't change the outcome. To prevent confusion, hide the wait button when the error is not triggered by the external bus timeout.
 - Have the refresh button trigger refresh errors instead of timeouts by using the correct url + function instead of interacting with WebView directly (for previous commit to work as expected)
 - Make the text on the refresh button and the action do the same thing by using the same check for internal
@Roffild
Copy link
Contributor

Roffild commented May 18, 2024

I also started with this simple fix. It doesn't fix ALL problems. The SETTINGS button needs to be fixed too.

I hid WAIT. It's horrible. Android moves buttons. You see 3 buttons. After 10 seconds you see 2 buttons in a different order. A missed click is guaranteed.

Accept my PR. I've already walked this path.

@Roffild
Copy link
Contributor

Roffild commented May 18, 2024

After 10 seconds again:

bug

@dshokouhi
Copy link
Member

I also started with this simple fix. It doesn't fix ALL problems.

in order to fix things iteratively we need to go step by step, we won't be able to fix all issues in a single PR but we can begin to walk through the steps and determine what's appropriate

The SETTINGS button needs to be fixed too.

we should not attempt to fix multiple things here and also users do indeed need to access Settings so not sure why you say it needs to be fixed, its doing what its intended to do, take users to settings to adjust things like their URL.

Accept my PR.

As mentioned in the previous PR please do not demand that PRs get merged and accepted, there is a process to follow and your PR still has lots of open comments that you did not address.

Copy link
Member

@dshokouhi dshokouhi left a comment

Choose a reason for hiding this comment

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

Works as described in my testing, the wait button gets hidden if server is down while attempting to be accessed

@Roffild
Copy link
Contributor

Roffild commented Jun 7, 2024

Why hide the button? To annoy the user?

The pop-up does not update automatically:

// Line 1287:
if (isShowingError || !isStarted || isRelaunching) {
    return
}
isShowingError = true

"WAIT" is hidden only from WebViewPresenterImpl.onGetExternalAuth().
waitForConnection() is almost always called earlier:

// Line 1414:
if (url != null) {
    loadUrl(url = url, keepHistory = true, openInApp = true) // <= waitForConnection :D
} else {
    waitForConnection()
}
////////////////
private fun waitForConnection() {
           showError(errorType = ErrorType.TIMEOUT_EXTERNAL_BUS) // <=
}

This patch does not work!

@Roffild
Copy link
Contributor

Roffild commented Jun 7, 2024

I am a professional programmer. I really need a working application.
You don't want or can't reproduce all the error variants from the server.

I tested all the variants. I looked at the Android source code. I would write UnitTests, but there are no tests. (Why?!)

Maybe you can trust me?

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