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

Pass custom plugins to BridgeFragment #3280

Merged
merged 2 commits into from
Jul 20, 2020
Merged

Pass custom plugins to BridgeFragment #3280

merged 2 commits into from
Jul 20, 2020

Conversation

fkirc
Copy link
Contributor

@fkirc fkirc commented Jul 17, 2020

The purpose of BridgeFragment is to integrate Capacitor into existing Android apps in cases where BridgeActivity would not fit into the existing native UI.
This is nice because it enables a gradual transition from a native codebase to Capacitor.

However, it is unclear how custom plugins should be passed to BridgeFragment.
Therefore, this PR introduces a new method to mimic the usage pattern of BridgeActivity.

@imhoffd imhoffd requested a review from carlpoole July 17, 2020 21:06
@carlpoole
Copy link
Member

Thanks for contributing. You are right, we do need a way to pass plugins into BridgeFragment. In BridgeActivity we do pass plugins in through the init method but in BridgeFragment this method is called automatically from onActivityCreated. Can you demonstrate how you were thinking this would be used in an app?

Our current train of thought was letting people set the plugins from newInstance or a setInitialPlugins method before the fragment gets inflated, like in this exploratory commit: 173458e

Example of use:

List<Class<? extends Plugin>> myPlugins = new ArrayList<>();
myPlugins.add(MyCustomPlugin.class);
myPlugins.add(MyCustomPlugin.class);
myPlugins.add(MyCustomPlugin.class);
        
BridgeFragment myEmbeddedFragment2 = BridgeFragment.newInstance("app1", myPlugins);

fragmentManager
		.beginTransaction()
		.replace(R.id.fragmentSpace, myEmbeddedFragment).commit();

or

BridgeFragment myEmbeddedFragment = BridgeFragment.newInstance("app1");
myEmbeddedFragment.addPlugin(MyCustomPlugin.class);

fragmentManager
		.beginTransaction()
		.replace(R.id.fragmentSpace, myEmbeddedFragment).commit();

@fkirc
Copy link
Contributor Author

fkirc commented Jul 20, 2020

Thanks for contributing. You are right, we do need a way to pass plugins into BridgeFragment. In BridgeActivity we do pass plugins in through the init method but in BridgeFragment this method is called automatically from onActivityCreated. Can you demonstrate how you were thinking this would be used in an app?

My usage pattern is slightly different to your snippets because I do not use FragmentManager to instantiate my fragment.
Instead, I use an XML-layout like this to instantiate my fragment:

<fragment android:id="@+id/my_fragment_id"
              tools:layout="@layout/my_fragment"/>

Therefore, the intended usage of this PR is to override the onCreate-method of BridgeFragment as demonstrated by the following code snippet. This usage pattern is very similar to the MainActivity: BridgeActivity in Capacitor's Android template:

import android.os.Bundle
import com.getcapacitor.BridgeFragment

class MyFragment : BridgeFragment() {

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        super.addPlugin(MyCapacitorPlugin::class.java)
    }
}

Our current train of thought was letting people set the plugins from newInstance or a setInitialPlugins method before the fragment gets inflated, like in this exploratory commit: 173458e

Example of use:

List<Class<? extends Plugin>> myPlugins = new ArrayList<>();
myPlugins.add(MyCustomPlugin.class);
myPlugins.add(MyCustomPlugin.class);
myPlugins.add(MyCustomPlugin.class);
        
BridgeFragment myEmbeddedFragment2 = BridgeFragment.newInstance("app1", myPlugins);

fragmentManager
		.beginTransaction()
		.replace(R.id.fragmentSpace, myEmbeddedFragment).commit();

This suggestions only supports the FragmentManager use case but not the XML use case.
Moreover, it might be buggy to pass plugins to a fragment like this.
Imagine if the Android OS destroys the BridgeFragment and then re-creates the BridgeFragment with its empty constructor.
If the plugins are not re-initialised correctly, then they might get lost after fragment re-creations (leading to all kind of runtime errors in the JavaScript code).

BridgeFragment myEmbeddedFragment = BridgeFragment.newInstance("app1");
myEmbeddedFragment.addPlugin(MyCustomPlugin.class);

fragmentManager
		.beginTransaction()
		.replace(R.id.fragmentSpace, myEmbeddedFragment).commit();

I think that the addPlugin-proposal is a good solution 👍
The reason is that the addPlugin method supports both the FragmentManager use case and the XML use case.
In other words, the non-static method addPlugin provides more flexibility than the static method newInstance.
Therefore, I rewrote this PR to add an addPlugin-method to BridgeFragment.

@fkirc
Copy link
Contributor Author

fkirc commented Jul 20, 2020

@carlpoole I have now rewritten this PR to implement your second proposal instead of my initial proposal.

@carlpoole
Copy link
Member

This is great feedback, thank you!

My usage pattern is slightly different to your snippets because I do not use FragmentManager to instantiate my fragment.
Instead, I use an XML-layout like this to instantiate my fragment:

<fragment android:id="@+id/my_fragment_id"
              tools:layout="@layout/my_fragment"/>

Therefore, the intended usage of this PR is to override the onCreate-method of BridgeFragment as demonstrated by the following code snippet. This usage pattern is very similar to the MainActivity: BridgeActivity in Capacitor's Android template:

import android.os.Bundle
import com.getcapacitor.BridgeFragment

class MyFragment : BridgeFragment() {

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        super.addPlugin(MyCapacitorPlugin::class.java)
    }
}

This should be fine. I think the addPlugin method can probably go anywhere in the lifecycle methods, as long as it is before the load method that gets called in the BridgeFragment onActivityCreated lifecycle event. (Where they get added to the Bridge and loaded in).

This suggestions only supports the FragmentManager use case but not the XML use case.
Moreover, it might be buggy to pass plugins to a fragment like this.
Imagine if the Android OS destroys the BridgeFragment and then re-creates the BridgeFragment with its empty constructor.
If the plugins are not re-initialised correctly, then they might get lost after fragment re-creations (leading to all kind of runtime errors in the JavaScript code).

I agree we should be careful around the potential for the re-creation of fragments in the system. Further changes are likely as 3.x matures and discovery of bugs around the embedded use case crop up.

@imhoffd imhoffd merged commit d131a5f into ionic-team:main Jul 20, 2020
kheftel pushed a commit to kheftel/capacitor that referenced this pull request Jul 22, 2020
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.

3 participants