[Web] Add a method to expose WebViewClient and WebChromeClient #1097

bentrengrove · 2022-03-21T06:43:59Z

Currently Accompanist Web sets it's own clients and doesn't provide a way to customise them. This limits the functionality and has created some obvious missing features. This is an attempt to expose the clients whilst maintaining the existing functionality. I did this via subclassing as I couldn't think of another way to do it, the clients themselves are classes not interfaces which limited the possibilities.

Open to other suggestions.

The other thing is I removed the onError callback as it was now duplicating functionality. I decided not to mark it as deprecated because this library was never released in a non-alpha version but I can create a deprecated version of it if needed.

manuelvicnt

Some nits! Overall LGTM :) thanks!

manuelvicnt · 2022-03-29T05:15:56Z

sample/src/main/java/com/google/accompanist/sample/webview/BasicWebViewSample.kt

@@ -115,13 +119,22 @@ class BasicWebViewSample : ComponentActivity() {
                        )
                    }

+                    // A custom WebViewClient and WebChromeClient can be provided via subclassing
+                    val webClient = object : AccompanistWebViewClient() {


Nit: this webClient should be remembered

manuelvicnt · 2022-03-29T05:25:29Z

web/src/main/java/com/google/accompanist/web/WebView.kt

+ * As Accompanist Web needs to set its own web client to function, it provides this intermediary
+ * class that can be overriden if further custom behaviour is required.
+ */
+open class AccompanistWebViewClient : WebViewClient() {


Sad truth: I wish we could make these interfaces 😞

manuelvicnt · 2022-03-29T05:27:08Z

web/src/androidTest/kotlin/com/google/accompanist/web/WebTest.kt

@@ -467,6 +529,8 @@ private fun WebTestContent(
    webViewState: WebViewState,
    idlingResource: WebViewIdlingResource,
    navigator: WebViewNavigator = rememberWebViewNavigator(),
+    client: AccompanistWebViewClient = AccompanistWebViewClient(),


Please remember the default value

manuelvicnt · 2022-03-29T05:28:40Z

web/src/androidTest/kotlin/com/google/accompanist/web/WebTest.kt

@@ -467,6 +529,8 @@ private fun WebTestContent(
    webViewState: WebViewState,
    idlingResource: WebViewIdlingResource,
    navigator: WebViewNavigator = rememberWebViewNavigator(),
+    client: AccompanistWebViewClient = AccompanistWebViewClient(),


Please remember this client

manuelvicnt · 2022-03-29T05:28:40Z

web/src/androidTest/kotlin/com/google/accompanist/web/WebTest.kt

@@ -467,6 +529,8 @@ private fun WebTestContent(
    webViewState: WebViewState,
    idlingResource: WebViewIdlingResource,
    navigator: WebViewNavigator = rememberWebViewNavigator(),
+    client: AccompanistWebViewClient = AccompanistWebViewClient(),
+    chromeClient: AccompanistWebChromeClient = AccompanistWebChromeClient()


Ditto: remember

manuelvicnt · 2022-03-29T05:33:53Z

web/src/androidTest/kotlin/com/google/accompanist/web/WebTest.kt

@@ -467,6 +529,8 @@ private fun WebTestContent(
    webViewState: WebViewState,
    idlingResource: WebViewIdlingResource,
    navigator: WebViewNavigator = rememberWebViewNavigator(),
+    client: AccompanistWebViewClient = AccompanistWebViewClient(),
+    chromeClient: AccompanistWebChromeClient = AccompanistWebChromeClient()


Ditto: remember

manuelvicnt · 2022-03-29T05:33:53Z

web/src/main/java/com/google/accompanist/web/WebView.kt

@@ -68,7 +68,8 @@ fun WebView(
    captureBackPresses: Boolean = true,
    navigator: WebViewNavigator = rememberWebViewNavigator(),
    onCreated: (WebView) -> Unit = {},
-    onError: (request: WebResourceRequest?, error: WebResourceError?) -> Unit = { _, _ -> }
+    client: AccompanistWebViewClient = AccompanistWebViewClient(),


Ditto: remember these two

manuelvicnt · 2022-03-29T05:33:53Z

web/src/androidTest/kotlin/com/google/accompanist/web/WebTest.kt

@@ -467,6 +529,8 @@ private fun WebTestContent(
    webViewState: WebViewState,
    idlingResource: WebViewIdlingResource,
    navigator: WebViewNavigator = rememberWebViewNavigator(),
+    client: AccompanistWebViewClient = AccompanistWebViewClient(),


Please remember this client

manuelvicnt · 2022-03-29T05:34:22Z

web/src/main/java/com/google/accompanist/web/WebView.kt

+ * class that can be overriden if further custom behaviour is required.
+ */
+open class AccompanistWebViewClient : WebViewClient() {
+    internal lateinit var state: WebViewState


Make these two open so that they can be intercepted by another impl?

I really don't want people to be able to go webViewClient.state = someOtherState when using it so maybe?

open lateinit var state: WebViewState internal set

manuelvicnt · 2022-03-29T05:34:34Z

web/src/main/java/com/google/accompanist/web/WebView.kt

+ * class that can be overriden if further custom behaviour is required.
+ */
+open class AccompanistWebChromeClient : WebChromeClient() {
+    internal lateinit var state: WebViewState


Ditto: consider open so that it can be intercepted

sampengilly · 2022-03-29T10:39:10Z

I think there's a question that comes up with opening this stuff up. Namely, should the compose state behaviour be guaranteed at all times, with the user only able to "hook-in" on top of that? Or should everything be able to be overridden by the user, including the default compose-like behaviours?

I also struggled a bit thinking about what the best way to expose the additional callbacks would be. The only other solution I had in mind outside of subclassing was to have something like the following:

class WebChromeClientCallbacks(
    val onProgressStarted: (...) -> Unit,
    val onProgressFinished: (...) -> Unit,
    ...
)

@Composable
fun rememberWebChromeClientCallbacks(
    onProgressStarted: (...) -> Unit = {},
    onProgressFinished: (...) -> Unit = {},
    ...
) = remember(...) { WebChromeClientCallbacks(...) }

// Same for WebViewClient

@Composable
fun WebView(
    ...
    webChromeClientCallbacks: WebChromeClientCallbacks = rememberWebChromeClientCallbacks()
) {
   // When setting up the WebChromeClient and WebViewClient, call 
   // the appropriate callback after handling the more compose-like behaviours
}

That would fall into the "compose-like behaviours are guaranteed, user can hook-in for anything else on top of that" approach. Similarly if the subclassing method somehow required a super call.

bentrengrove · 2022-03-29T21:59:27Z

The subclasses will be required to call super but that applies to when you use webview without Compose as well I think. I did think about doing the callbacks in a class idea as well, that's how Maps Compose did it, it just seemed like there were so many that we may as well match the implementation of webview.

I am happy with this approach for now but we can change it if we get more feature requests/bugs for things people can't do.

Doomsdayrs · 2022-03-29T23:19:11Z

Could this not have been accomplished with a "configure webviewclient" function?

sampengilly · 2022-03-29T23:29:31Z

Just had a thought, wouldn't this also need to set the clients in the update callback of AndroidView() to cover any changes over time?

Add a method to expose WebViewClient and WebChromeClient

f925bd0

bentrengrove requested a review from manuelvicnt March 21, 2022 06:43

bentrengrove changed the title ~~Add a method to expose WebViewClient and WebChromeClient~~ [Web] Add a method to expose WebViewClient and WebChromeClient Mar 22, 2022

manuelvicnt reviewed Mar 29, 2022

View reviewed changes

bentrengrove added 3 commits March 29, 2022 16:42

Add missing remembers

b038002

Update internal variables to internal set

cdce380

Update API

02961b0

sampengilly mentioned this pull request Mar 29, 2022

[Web] Handle WebView Full Screen callbacks via state #1113

Closed

manuelvicnt approved these changes Mar 29, 2022

View reviewed changes

bentrengrove merged commit c3af5e8 into main Mar 29, 2022

bentrengrove deleted the ben/expose_web_client branch March 29, 2022 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Web] Add a method to expose WebViewClient and WebChromeClient #1097

[Web] Add a method to expose WebViewClient and WebChromeClient #1097

bentrengrove commented Mar 21, 2022

manuelvicnt left a comment

manuelvicnt Mar 29, 2022

manuelvicnt Mar 29, 2022

manuelvicnt Mar 29, 2022

manuelvicnt Mar 29, 2022

manuelvicnt Mar 29, 2022

manuelvicnt Mar 29, 2022

manuelvicnt Mar 29, 2022

manuelvicnt Mar 29, 2022

manuelvicnt Mar 29, 2022

bentrengrove Mar 29, 2022 •

edited

manuelvicnt Mar 29, 2022

sampengilly commented Mar 29, 2022 •

edited

bentrengrove commented Mar 29, 2022

Doomsdayrs commented Mar 29, 2022

sampengilly commented Mar 29, 2022

[Web] Add a method to expose WebViewClient and WebChromeClient #1097

[Web] Add a method to expose WebViewClient and WebChromeClient #1097

Conversation

bentrengrove commented Mar 21, 2022

manuelvicnt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bentrengrove Mar 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sampengilly commented Mar 29, 2022 • edited

bentrengrove commented Mar 29, 2022

Doomsdayrs commented Mar 29, 2022

sampengilly commented Mar 29, 2022

bentrengrove Mar 29, 2022 •

edited

sampengilly commented Mar 29, 2022 •

edited