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

[Web] Fix webview not destroyed #1221

Merged
merged 4 commits into from Jul 4, 2022
Merged

[Web] Fix webview not destroyed #1221

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2985c8c
Add onDispose param
rlatapy-luna Jun 30, 2022
4d20e8a
Add minimal test testOnDisposeCalled
rlatapy-luna Jul 1, 2022
fc358b6
Update API
rlatapy-luna Jul 1, 2022
a3cc01f
Remove default destroy call
rlatapy-luna Jul 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions web/src/main/java/com/google/accompanist/web/WebView.kt
View file
Open in desktop
Expand Up @@ -25,6 +25,7 @@ import android.webkit.WebView
import android.webkit.WebViewClient
import androidx.activity.compose.BackHandler
import androidx.compose.runtime.Composable
import androidx.compose.runtime.DisposableEffect
import androidx.compose.runtime.Immutable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.Stable
Expand All @@ -33,6 +34,7 @@ import androidx.compose.runtime.mutableStateListOf
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberCoroutineScope
import androidx.compose.runtime.rememberUpdatedState
import androidx.compose.runtime.setValue
import androidx.compose.ui.Modifier
import androidx.compose.ui.platform.LocalInspectionMode
Expand Down Expand Up @@ -68,6 +70,7 @@ fun WebView(
captureBackPresses: Boolean = true,
navigator: WebViewNavigator = rememberWebViewNavigator(),
onCreated: (WebView) -> Unit = {},
onDispose: (WebView) -> Unit = { it.destroy() },
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't mind exposing the callback but I think it.destroy() should be moved to the DisposableEffect onDispose and always called. Is there a valid usecase for not calling it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about configuration changes. But it probably requires more work to handle it properly (cf #1178).

client: AccompanistWebViewClient = remember { AccompanistWebViewClient() },
chromeClient: AccompanistWebChromeClient = remember { AccompanistWebChromeClient() }
) {
Expand All @@ -81,6 +84,14 @@ fun WebView(
with(navigator) { webView?.handleNavigationEvents() }
}

val currentOnDispose by rememberUpdatedState(onDispose)

webView?.let { it ->
DisposableEffect(it) {
onDispose { currentOnDispose(it) }
}
}

// Set the state of the client and chrome client
// This is done internally to ensure they always are the same instance as the
// parent Web composable
Expand Down