Add support for activity scoped navigation request handlers #125
Conversation
| @@ -9,6 +9,8 @@ | |||
| import androidx.fragment.app.Fragment; | |||
| import androidx.fragment.app.FragmentManager; | |||
|
|
|||
| import com.ern.api.impl.navigation.NavigationRouteHandler; | |||
There was a problem hiding this comment.
Hmm, so this creates a dependency from .core to .navigation. I'm pretty sure we already have several other such references, and there's already a circular dependency between these two packages.
No action item right now, but we need to remember for "Version 2.0" to rethink the package/dependency structure we're using here. Java package constraints can also be enforced through unit test via tools like JDepend. Something we can consider eventually.
There was a problem hiding this comment.
Very good find, this was an oversight. There shouldn't be this dependency. But I agree, looking at how this is being used we may no longer need a core package as this was initially kept for backward compatibility but no one is actually using it.
I will fix the dependency now. Thanks.
| */ | ||
| boolean mUseActivityScopedNavigation; | ||
|
|
||
|
|
| * When true: {@link com.ern.api.impl.navigation.ReactNavigationViewModel} will be created with activity scope instead of fragment scope. | ||
| * The activity will then delegate the navigation requests to the corresponding fragments via {@link com.ern.api.impl.navigation.NavigationRouteHandler} implementation. | ||
| * The {@link ElectrodeBaseActivityDelegate} will take the current instance of the {@link Fragment} that is loaded inside {@link LaunchConfig#getFragmentContainerId()}. Assumption is that this fragment implements {@link NavigationRouteHandler} | ||
| * Pass {@link RouteHandlerProvider} If you would like to provide your own implementation of providing the fragment instance, valid only if #value is true. |
There was a problem hiding this comment.
No real action here, but a todo for an eventual cleanup: You probably noticed that these five lines are a one-to-one copy+paste from lines 97-101.
deepueg
force-pushed
the
add-nav-support-activity-scope
branch
from
ef6e29d to
dccac98
Compare
This gives more flexibility for applications that follows a complex UI design
deepueg
force-pushed
the
add-nav-support-activity-scope
branch
from
dccac98 to
5b0e5ab
Compare
There was a problem hiding this comment.
The imports look a lot cleaner now. Nice! 👍
I assume you tested all possible scenarios locally? I'm sure you also have the best knowledge how this change will affect the public API and what needs to be done in terms of versioning. I already approved, nothing blocking from my side!
| mRouteHandlerProvider = routeHandlerProvider; | ||
| } | ||
|
|
||
| public interface RouteHandlerProvider<T extends Fragment & NavigationRouteHandler> { |
There was a problem hiding this comment.
I just learned something new. Fascinating 🤯
There was a problem hiding this comment.
yeah, its pretty neat to have this at compile time.
There was a problem hiding this comment.
The imports look a lot cleaner now. Nice! 👍
I assume you tested all possible scenarios locally? I'm sure you also have the best knowledge how this change will affect the public API and what needs to be done in terms of versioning. I already approved, nothing blocking from my side!
Yes, this change does not really impact any existing APIs. It's adding a new delegate and will only be available for those who add this explicitly. Validated for backward compatibility.
This gives more flexibility for applications that follows a complex UI design