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

[Web] Fix webview not destroyed #1221

Merged
merged 4 commits into from Jul 4, 2022
Merged

Conversation

rlatapy-luna
Copy link
Contributor

@rlatapy-luna rlatapy-luna commented Jun 30, 2022

Fix #1220

Give access to the webview during composable dispose run. By default, destroy the webview.

Use https://webbrowsertools.com/audio-test/ for testing (see #1220)

Destroy webview by default on dispose
Copy link
Collaborator

@bentrengrove bentrengrove left a comment

Thank you for the bug report and the PR to fix it!

Could you please also add a basic test to ensure onDispose is called?

@@ -68,6 +70,7 @@ fun WebView(
captureBackPresses: Boolean = true,
navigator: WebViewNavigator = rememberWebViewNavigator(),
onCreated: (WebView) -> Unit = {},
onDispose: (WebView) -> Unit = { it.destroy() },
Copy link
Collaborator

@bentrengrove bentrengrove Jul 1, 2022

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

@rlatapy-luna rlatapy-luna Jul 1, 2022

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).

@bentrengrove
Copy link
Collaborator

bentrengrove commented Jul 1, 2022

You'll also have to run

./gradlew metalavaGenerateSignature to update the API file

@rlatapy-luna
Copy link
Contributor Author

rlatapy-luna commented Jul 1, 2022

I added the minimal test, tell me if you prefer to not expose the onDispose callback

@rlatapy-luna rlatapy-luna requested a review from bentrengrove Jul 1, 2022
@bentrengrove
Copy link
Collaborator

bentrengrove commented Jul 4, 2022

That is a very good point about making that linked issue worse. This might actually be too big a breaking change to introduce in the rc stage. For now, let's remove the it.destroy from the callback and just expose the callback as an empty callback.

Once we are back to alphas, we can investigate where is the best place to call it. Anyone on 1.2 can use the new callback to call it themselves manually if required.

Copy link
Collaborator

@bentrengrove bentrengrove left a comment

Thank you!

@bentrengrove bentrengrove enabled auto-merge Jul 4, 2022
@rlatapy-luna
Copy link
Contributor Author

rlatapy-luna commented Jul 4, 2022

@bentrengrove Tests are not failing on my side 🤷‍♂️

@bentrengrove bentrengrove merged commit 983ff4c into google:main Jul 4, 2022
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants