ionic-team / ionic-portals-android

Other
4 stars 3 forks source link

PortalView has an unnecessary dependency on AppCompatActivity, and fails without throwing a proper exception. #47

Closed mattmckeon closed 6 months ago

mattmckeon commented 1 year ago

PortalView requires its containing activity to implement AppCompatActivity. This is bad for applications using Jetpack Compose, because AppCompatActivity requires you to use a theme (AppCompatTheme) that is incompatible with the recommended Material theme in Compose apps.

This is an overly restrictive approach, for two reasons:

  1. The only method PortalView uses from AppCompatActivity, [getSupportFragmentManager](https://developer.android.com/reference/androidx/fragment/app/FragmentActivity#getSupportFragmentManager()), is actually from AppCompatActivity's base class, FragmentActivity. The cast here could be to FragmentActivity, which is from androidx.fragment and has a minSDK of 14 vs Portals overall min SDK of 22.
  2. Because all that's required is the FragmentManager, PortalView could optionally take a Lazy-valued FragmentManager as a required constructor parameter. This would be first evaluated at load time, after the view has been attached, and remove the dependency on a particular implementation of Activity.

Finally, if after all this the PortalView is still in an invalid context, the code should check the cast and throw a proper exception with an informative error message, as opposed to a class cast exception.

Steven0351 commented 1 year ago

Our Android dev @carlpoole is out at the moment and is the only person who can speak intelligently on the points you've brought up. Regarding the min SDK comment, Portals follows the minimum SDK requirements of Capacitor.

carlpoole commented 1 year ago

Thanks for the feedback. The AppCompatActivity is a constraint currently required for core dependency Capacitor and Cordova

Our team works closely with Capacitor and we have thoughts on rearchitecting the project at large to allow for Portals to support the Material compose theme

carlpoole commented 6 months ago

Current project depends on Capacitor constraints.