B3nedikt / ViewPump

View Inflation you can intercept.
Apache License 2.0
165 stars 15 forks source link

[Bug][Feature request][Crash] - Compose support #22

Closed iTruff closed 3 years ago

iTruff commented 3 years ago

Hi @B3nedikt For now ViewPump is not working for fragments created like this:

class ComposeFragment : Fragment() {

    override fun onCreateView(
        inflater: LayoutInflater,
        container: ViewGroup?,
        savedInstanceState: Bundle?
    ): View? {
        return ComposeView(requireContext()).apply {
            setContent {
                MxdTheme {
                    setUpComposable()
                }
            }
        }
    }

    /**
     * Should be implemented by child views to set up [Composable].
     */
    @Composable
    abstract fun setUpComposable()
}

ViewPumpAppCompatDelegate.onCreateView methods are not even called. If setUpComposable() contains AndroidView with WebView inside, app will crash due to wrong resources passed to the it.

@Composable
fun setUpComposable() {
    Box {
        AndroidView(
            factory = {
                var context = it

                // Here we are getting modified context and resources
                Timber.d("webViewFactory: context=$context")
                Timber.d("webViewFactory: resources=${context.resources}")

                webViewFactory(
                    context = context,
                    onRedirectAttempt = onRedirectAttempt,
                    onRedirectStarted = onRedirectStarted,
                    onRedirectProgressChanged = onRedirectProgress,
                    onRedirectFinished = onRedirectFinished
                )
            },
            update = {
                webViewUpdate(
                    webView = it,
                    userAgent = userAgent,
                    url = url
                )
            }
        )

        if (progress != 1.0f) {
            LinearProgressIndicator(
                modifier = Modifier
                    .height(2.dp)
                    .fillMaxWidth(),
                progress = progress,
                color = Color(0xFFE20019)
            )
        }
    }
}

/**
 * The factory block will be called exactly once to obtain the View to be composed, and it is also
 * guaranteed to be invoked on the UI thread. Therefore, in addition to creating the factory, the
 * block can also be used to perform one-off initializations and View constant properties' setting.
 */
private fun webViewFactory(
    context: Context,
    onRedirectAttempt: (url: String) -> Boolean,
    onRedirectStarted: (url: String) -> Unit,
    onRedirectProgressChanged: (progress: Int) -> Unit,
    onRedirectFinished: (url: String) -> Unit
): WebView {
    return WebView(context).apply {
        settings.javaScriptEnabled = true
        webViewClient = object : WebViewClient()
        webChromeClient = object : WebChromeClient()
    }
}

/**
 * The update block can be run multiple times (on the UI thread as well) due to recomposition,
 * and it is the right place to set View properties depending on state. When state changes,
 * the block will be reexecuted to set the new properties. Note the block will also be ran once
 * right after the factory block completes.
 */
private fun webViewUpdate(
    webView: WebView,
    userAgent: String,
    url: String
) {
    webView.setUserAgent(userAgent)
    webView.loadUrl(url)
}

Do you have any plans to add Compose support? If not, could you please at least give some advises on how to fix this?

iTruff commented 3 years ago

@B3nedikt After small research seems like the problem is not with ViewPump but with AppLocale, checking how solve...

B3nedikt commented 3 years ago

Hi @iTruff :) The methods from ViewPumpAppCompatDelegate.onCreateView are not called, as compose does not use normal view inflation. The idea behind ViewPump is to intercept view inflation, which does not happen when using compose. So I am not sure if this would be a good feature. Maybe mixed compose/legacy view system projects are a use case. Maybe I will create a separate lib for compose in the future.

If you just want to fix the resources for the web view you can however just do:

 val fixedContext = (requireActivity() as AppCompatActivity).delegate.attachBaseContext2(contextYouUseForWebView)
iTruff commented 3 years ago

@B3nedikt Thanks for the quick response, I've tried above solution but it doesn't work. In my base activity I have following code:

abstract class LocaleAwareActivity<T : BaseMvvmViewModel>(
    @LayoutRes contentLayoutId: Int
) : BaseMvvmActivity<T>(contentLayoutId) {
    /**
     * Wrap [AppCompatDelegate] with [ViewPumpAppCompatDelegate] to update locale dynamically.
     */
    private val appCompatDelegate: AppCompatDelegate by lazy {
        ViewPumpAppCompatDelegate(
            baseDelegate = super.getDelegate(),
            baseContext = this,
            wrapContext = { baseContext ->
                AppLocale.wrap(baseContext)
            }
        )
    }

    /**
     * Return wrapped [AppCompatDelegate].
     */
    override fun getDelegate(): AppCompatDelegate {
        return appCompatDelegate
    }
}

AppLocale.wrap(baseContext) cause problems, if I remove this line, WebView works correctly. And I still can't understand how can I properly unwrap this context.

B3nedikt commented 3 years ago

You're welcome :) Yeah, wrapping the context at this point will lead to the crash, as you create the WebView with the factory in compose, not using the LayoutInflater. Can you try to modify the test app of this repository, so it reproduces the problem? Just create a fork of this repository, and link the modified fork to this issue in a comment.

iTruff commented 3 years ago

Ok, I will, hope we will find solution :)

iTruff commented 3 years ago

@B3nedikt I've added reproduceable sample here.

To reproduce crash you just need to click on Open compose activity button, WenView with test page will be rendered. After clicking on select tag (html version of the Android's dropdown) - app will crash.

https://pasteboard.co/nF4DWbVRIR31.png

Do you need something else?

B3nedikt commented 3 years ago

No, thanks this is enough to understand your problem :)

Fixing this issue in a nice way is actually quite difficult, the main problem is that you would ideally create the WebView without using the LayoutInflater. But then you will never be able to get a wrapped context inside the dialog without crashing, as the webview changes the resources. In short the context we wrap and the context used for inflating the views in the dialog shown by the web view using legacy views needs to be the same, as the webview will always use a LayoutInflater for the views of the dialog.

Anyway for getting around the crash the following workaround is okay:

Just create the webview the old way as a web_view.xml file like this:

<WebView xmlns:android="http://schemas.android.com/apk/res/android"
    android:layout_width="match_parent"
    android:layout_height="match_parent" />

And inflate it in compose like this:

/**
 * The factory block will be called exactly once to obtain the View to be composed, and it is also
 * guaranteed to be invoked on the UI thread. Therefore, in addition to creating the factory, the
 * block can also be used to perform one-off initializations and View constant properties' setting.
 */
@SuppressLint("SetJavaScriptEnabled")
private fun webViewFactory(context: Context): WebView {
    return (LayoutInflater.from(context).inflate(R.layout.web_view, null) as WebView).apply {
        settings.javaScriptEnabled = true
        webViewClient = object : WebViewClient() {
            override fun shouldOverrideUrlLoading(view: WebView, url: String): Boolean {
                return false
            }

            override fun shouldOverrideUrlLoading(
                view: WebView,
                request: WebResourceRequest
            ): Boolean {
                return false
            }
        }
    }
}

Not ideal, but I can't think of a better solution for now ;)

iTruff commented 3 years ago

@B3nedikt Wow, that was fast :D Cool when you know a lot about Context in Android :) Just checked your solution, works like a charm, thanks a lot. For now I'll keep it as is, will keep you posted in case of better solution.

B3nedikt commented 3 years ago

Good to know, feel free to reopen when you find a better solution :)